Skip to content

fix(gateway): scope session-only /model context_length to configured route - #48187

Open
ele-yufo wants to merge 5 commits into
NousResearch:mainfrom
ele-yufo:fix/route-scoped-context-length
Open

fix(gateway): scope session-only /model context_length to configured route#48187
ele-yufo wants to merge 5 commits into
NousResearch:mainfrom
ele-yufo:fix/route-scoped-context-length

Conversation

@ele-yufo

Copy link
Copy Markdown

Summary

Fix session-scoped /model context-length resolution so a global model.context_length override does not bleed into unrelated runtime routes.

Symptoms

With this config:

model:
  default: glm-5.2
  provider: zai
  context_length: 1000000

a session-only gateway switch like:

/model gpt-5.5 --provider openai-codex

previously showed 1,000,000 tokens, even though Codex OAuth gpt-5.5 should resolve through the provider-aware path (272K in current Hermes metadata).

What this PR changes

Only the gateway confirmation path for /model is changed.

Added _config_context_length_for_target() in gateway/slash_commands.py to return the global model.context_length override only when it belongs to the same configured route (model.default + model.provider).

Both call sites that previously unconditionally passed the global model.context_length into resolve_display_context_length() now call this guard instead.

What this PR intentionally does NOT change

  • No change to agent_init.py / global default semantics.
  • No change to CLI picker logic.
  • No change to Codex context metadata itself.
  • No attempt to rewrite general provider/context resolution.

This PR fixes the narrower route-mismatch bug for session-only /model switches.

Why this matters

Global context overrides are useful, but they are route-specific in practice. A 1M override for one provider/model should not silently transfer to a different provider route whose real enforced cap is different.

This matters especially for Codex routes, where provider-aware context caps differ from direct-API OpenRouter/OpenAI metadata.

Related upstream issues

This fix is closely related to, but narrower than, several existing open issues/PRs:

In particular, it addresses the session-only /model route mismatch path rather than the broader --global stale-override family.

Verification

  • New regression test added: tests/gateway/test_model_command_context_scope.py
  • Targeted tests run:
    • tests/gateway/test_model_command_context_scope.py
    • tests/gateway/test_model_command_flat_string_config.py
    • tests/hermes_cli/test_model_switch_context_display.py
    • tests/agent/test_model_metadata.py::TestCodexOAuthContextLength
    • tests/gateway/test_model_command_custom_providers.py
    • tests/gateway/test_model_switch_persistence.py
    • tests/hermes_cli/test_model_switch_custom_providers.py

Example local result during verification:

  • 25 passed in 87.88s
  • 35 passed in 112.88s

Scope discipline

  • Only gateway/slash_commands.py behavior and one new test file are included.
  • No unrelated runtime behavior is changed.

Notes

This PR intentionally does not close the broader --global stale model.context_length cleanup family; that is a larger, pre-existing upstream problem.

@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have labels Jun 18, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Approved

Scopes the session-only /model context_length to the configured route in the gateway, preventing cross-session context length bleed.

Looks Good

  • Clean fix, 2 files
  • Correct scoping behavior
  • No side effects

Reviewed by Hermes Agent

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for isolating both gateway confirmation paths. The route-mismatch premise is still present on current main: gateway/slash_commands.py:1686-1703 and gateway/slash_commands.py:1940-1957 pass the global override unconditionally.

Problems

  • tests/gateway/test_model_command_context_scope.py:79 does not include --session. Current resolve_persist_behavior() persists plain /model commands by default (hermes_cli/model_switch.py:416-443), so this no longer proves the stated session-only case.
  • Current main writes model.default and model.provider before confirmation construction (gateway/slash_commands.py:1897-1927; picker equivalent at 1650-1676). Reloading config in the proposed guard afterward sees the new target route and accepts the inherited context_length for persistent switches.

Suggested changes

  • Add --session to the regression and assert the original config route remains unchanged.
  • Salvage against current persistence ordering by using pre-switch route state for the session-only check, while keeping global behavior explicitly scoped.

Automated hermes-sweeper review.

monkeypatch.setattr("hermes_cli.model_switch.switch_model", lambda **kw: _codex_switch_result())
monkeypatch.setattr("hermes_cli.model_cost_guard.expensive_model_warning", lambda *a, **kw: None)

result = await _make_runner()._handle_model_command(_make_event("/model gpt-5.5 --provider openai-codex"))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add --session here. Current main persists a plain /model switch by default (hermes_cli/model_switch.py:416-443), so this command does not exercise the session-only behavior described by the test and PR.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/sessions Session lifecycle, resume, persistence, history labels Jul 14, 2026
yufo added 4 commits August 5, 2026 02:30
PR NousResearch#48187 hermes-sweeper review: the session-only regression should carry
--session and assert the configured route survives the switch, and the
pre-switch route-state guard should keep persistent switches explicitly
scoped.

The capture-at-start refactor on this branch already resolves both the
session-only display check and the global persist through pre-switch
config (should_clear_context_pin), so this adds the matching regression
coverage:

- the --session test now also asserts config.yaml is untouched afterward
- a new --global test proves the stale 1M context_length is neither shown
  (confirmation reports Codex's real 272K cap) nor inherited (the
  route-mismatched pin is dropped from the persisted config)
@ele-yufo

ele-yufo commented Aug 5, 2026

Copy link
Copy Markdown
Author

Follow-up after hermes-sweeper review (keep_open, salvageability=medium):

What changed:

  • Merged latest origin/main into the branch (no conflicts).
  • gateway/slash_commands.py: unchanged in this iteration — the capture-at-start refactor (snapshot configured_model/provider/base_url/context_length at the top of _handle_model_command, before any config writes) already addresses the sweeper's concern: the confirmation path no longer reloads config after model.default/provider are written, so the session-only guard evaluates the pre-switch route state. Global switches drop the stale context pin on route change instead of inheriting it.
  • tests/gateway/test_model_command_context_scope.py: the session-only regression now runs with --session and asserts the original config route (glm-5.2/zai/1M) is unchanged after the switch. Added test_global_model_switch_drops_stale_context_pin covering the persistent-switch path — verifies it does not inherit the 1M context length and that the persisted context_length is removed.

Test results: target file 2 passed; adjacent cluster (persist_default / global_switch_persists / apply_result_context / context_offload / custom_provider_context_length) 20 passed, 0 failed; ruff check + format clean.

@teknium1 could you re-run the sweeper against the updated head?

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

Labels

area/sessions Session lifecycle, resume, persistence, history comp/gateway Gateway runner, session dispatch, delivery P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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