Skip to content

fix(gateway): drop stale model/provider keys in session model_config (resume routed to wrong provider) - #96748

Closed
ahrazzle wants to merge 1 commit into
NousResearch:mainfrom
ahrazzle:fix/session-model-config-desync
Closed

ahrazzle wants to merge 1 commit into
NousResearch:mainfrom
ahrazzle:fix/session-model-config-desync

Conversation

@ahrazzle

@ahrazzle ahrazzle commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

What

_runtime_model_config (TUI/desktop gateway persist path) merges the agent's current identity onto the row's existing model_config JSON. For model and provider it only SET the key when the agent attribute was truthy — while base_url / api_mode / reasoning_config / service_tier already deleted stale values when falsy. When an agent rebuilt with an empty provider (inheriting the profile default) was persisted, the previous provider/endpoint survived in model_config while _persist_live_session_runtime updated the model column separately.

On resume, _stored_session_runtime_overrides reads the model from the column but the provider/endpoint from model_config — so the chat silently routed to the stale provider (e.g. a VeniceAI/empero endpoint) under a model that should run on the profile default.

This is the same stale-key bug class the CLI path already fixed with or-None deletion in _persist_model_switch_to_session (see tests/cli/test_resume_model_restore.py::test_persist_model_switch_clears_stale_route_keys); the gateway writer never got the symmetric treatment. The fix applies delete-on-falsy to model and provider too, matching the existing handling of the other identity keys.

Evidence (real incident)

During a bulk provider rotation across 8 profiles / 174 chat sessions, the audit found 4 live rows where sessions.model was already deepseek/deepseek-v4-flash-0731 but model_config still carried provider: stealth-ox-alpha (VeniceAI) / empero with base_url + api_mode. Config.yaml was correct; the desync was purely in the session rows, and would have routed those resumes to the wrong endpoint.

Reproduction

from tui_gateway.server import _runtime_model_config
from types import SimpleNamespace

agent = SimpleNamespace(model="deepseek/deepseek-v4-flash-0731", provider="",
                        base_url="", api_mode="", reasoning_config=None,
                        service_tier=None)
existing = {"model": "deepseek/deepseek-v4-flash-0731",
            "provider": "stealth-ox-alpha",      # stale
            "base_url": "https://api.venice.ai/api/v1", "api_mode": "chat_completions"}
config = _runtime_model_config(agent, existing)
# before: config["provider"] == "stealth-ox-alpha"  (stale survives)
# after:  "provider" not in config                  (deleted — resume falls back)

Tests

New TestRuntimeModelConfigDropsStaleKeys in tests/tui_gateway/test_custom_provider_session_persistence.py:

  • falsy provider / falsy model delete the stale existing key,
  • truthy provider overwrites stale existing,
  • resume overrides fall back to the billing provider instead of the stale endpoint,
  • real-DB round trip heals an already-desynced row on the next live persist,
  • first write (existing=None) reflects only the agent's current identity.

Verified green on the touched surface: 17 passed in the target file, plus tests/cli/test_resume_model_restore.py (22) and tests/state/ (89) — this change introduces no new failures. The target and sibling suites exercise the real resume path (real SessionDB, temp HERMES_HOME), not mocks.

Existing desynced rows

Self-heal on the next live metadata persist (any /model switch or reconnect writes through _runtime_model_config). No migration needed.

Related: #96745 (CLI for bulk session model audit/reset — the tooling gap this incident exposed).

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tui Terminal UI (ui-tui/ + tui_gateway/) area/sessions Session lifecycle, resume, persistence, history sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 28, 2026
_runtime_model_config merges the agent's current identity onto the row's
existing model_config JSON. For model and provider it only SET the key
when the agent attribute was truthy, while base_url/api_mode/
reasoning_config/service_tier already deleted stale values when falsy.
When an agent rebuilt with an empty provider (inheriting the profile
default) was persisted, the previous provider/endpoint survived in
model_config while _persist_live_session_runtime updated the model
column separately. Resume then read the fresh model from the column but
the STALE provider from model_config, silently routing the resumed chat
to the wrong endpoint (e.g. a VeniceAI/empero route under a model that
should run on the profile default).

Apply the same delete-on-falsy rule to model and provider, mirroring the
or-None deletion the CLI path (_persist_model_switch_to_session) already
uses, so a stale session state can never survive into a resume override.
Existing desynced rows self-heal on the next live metadata persist.

Adds regression tests: merge drops stale provider/model when the agent
attribute is falsy, a truthy provider overwrites the stale value, resume
overrides fall back to the billing provider instead of the stale
endpoint, a real-DB round trip heals an already-desynced row, and a
first write (existing=None) reflects only the agent's current identity.
@ahrazzle

Copy link
Copy Markdown
Contributor Author

Author's note — post-revision scope verification (single-commit state)

After trimming the branch to the surgical fix (registry/helper removed), I re-verified the exact behavioral scope. Three things worth flagging for maintainers:

1. The model deletion is defensive hygiene; the provider deletion is the actual fix.
_persist_live_session_runtime writes the model column via db.update_session_meta(session_key, json.dumps(model_config), model or None) and update_session_meta applies model = COALESCE(?, model) — so when agent.model is falsy, the column is left unchanged, and resume (_stored_session_runtime_overrides) reads the column first. Popping the JSON model key (server.py:5325) therefore never changes real resume routing; it only keeps the JSON consistent with the agent's live identity.

The provider half (server.py:5357) is the behavioral fix: there is no separate provider column, so deleting the stale JSON provider — and the endpoint keys that ride with it — is what unblocks the billing_provider/ambient fallback on resume. That is the exact incident class (4 desynced rows, 174 sessions) this PR closes.

Corollary: test_falsy_model_drops_stale_existing_model (tests line 391) is slightly over-strong — "an empty agent model cannot keep the row's old model as its own." The row's column still does; only the JSON no longer does. Harmless, but worth a docstring tweak if this file is touched again.

2. Unknown model_config keys are preserved by construction, but that property is now untested.
The merge copies existing and pops only the six managed keys, so lineage markers (_branched_from, _delegate_from) and keys written by newer builds survive every persist. This was the original motivation for the key-registry approach; with the registry gone, the property rests on construction, not on a test (the registry-dependent survival test was dropped with it). A 3-line test asserting unknown keys survive a _runtime_model_config merge would pin it permanently.

3. The nested gateway_runtime carrier is not a sibling hole.
_sync_session_model_from_agent (gateway/run.py) replaces gateway_runtime as a whole dict every turn with current agent state (empties filtered), so it self-heals the same stale-provider class by design and is unaffected by this change.

Verified: 128 passed (17 in the persistence file; tests/cli/test_resume_model_restore.py + tests/state/ green). The 4 new regression tests fail against pre-fix server.py (confirmed RED), so they genuinely pin the bug.

@ahrazzle

Copy link
Copy Markdown
Contributor Author

QA review summary (Team6 QA pass on this PR)

Reviewed against main and the committed branch, plus a local run of the touched suites.

Substance: correct and well-scoped.

  • The bug is real: on main, _runtime_model_config SET model/provider on truthy but had no else branch, so a falsy agent provider (inheriting the profile default) left the stale provider/endpoint in model_config while _persist_live_session_runtime wrote the model column separately — resume then read a fresh model but a stale provider. Confirmed both keys lacked the drop branch on main.
  • The fix mirrors the existing handling of the other identity keys (base_url/api_mode/reasoning_config/service_tier already pop on falsy): two else: config.pop(..., None) branches on model and provider. Minimal, idiomatic, no new surface.
  • Fixes the whole class; the PR body correctly notes the sibling CLI path (_persist_model_switch_to_session) already does the symmetric deletion, so this closes the remaining site rather than duplicating it.

Tests: good behavior contracts, not snapshots.

  • TestRuntimeModelConfigDropsStaleKeys asserts the invariant (stale dropped on falsy; truthy overwrites; resume falls back to the billing provider) and exercises the real restore path. test_real_db_persist_heals_desynced_row drives a real SessionDB under a temp HERMES_HOME. Matches the repo's "E2E validation, not green mocks" bar.

Verification I ran:

  • tests/tui_gateway/test_custom_provider_session_persistence.py → 17 passed
  • tests/cli/test_resume_model_restore.py → 22 passed
  • tests/state/ → 89 passed
  • No import/syntax breakage; module imports clean.

One housekeeping note (already corrected in the PR body):

  • The earlier "pre-existing failure in test_gui_surface_toolsets.py::test_holds_exactly_the_gui_affordances" claim is inaccurate — that test passes on both main and this branch and is orthogonal to the model/provider pop logic. Removed so the evidence section can't be challenged by a reviewer who runs the suite.

Scope is tight (bugfix only; the bulk-reset CLI is correctly kept in a separate issue #96745). No speculative infrastructure, no new env vars, no cache-breaking changes. Mergeable as-is.

novnski added a commit to novnski/hermes-agent that referenced this pull request Aug 28, 2026
Adopt reviewed ideas from upstream PRs NousResearch#96251, NousResearch#96498, NousResearch#96748, NousResearch#96884, and NousResearch#96885 with local integration tests.

Co-authored-by: loulanyue <260355617@qq.com>

Co-authored-by: 686f6c61 <github@00b.tech>

Co-authored-by: ahrazzle <ahraz.arifuddin@gmail.com>

Co-authored-by: Agi-Asi <Agi-Asi@users.noreply.github.com>
@teknium1

Copy link
Copy Markdown
Collaborator

Merged via PR #97066 (rebase-merge) — your commit landed on main with your authorship preserved in git log. The gateway metadata writer now drops stale model/provider keys with the same or-None semantics as the CLI path, so a falsy agent identity can no longer leave a wrong-but-routable provider in the session row. Your regression tests (including the real-DB desync heal) came along intact. Thanks for catching the write-side half of this bug class — the resume-side fix alone (#97008) would have left it reachable.

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/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants