fix: unify route dict or-None discipline in /model persist
The route dict in _persist_model_switch_to_session used filtering (omits falsy values) while the top-level keys used (writes explicit None to trigger deletion in _merge_model_config_json). This asymmetry meant stale keys from a previous /model switch survived in the nested gateway_runtime dict even after the fix in #85261 that properly deleted them from the top-level keys. Fix: build the route dict with and derive the top-level keys from **route so both shapes always use identical deletion semantics. Also filter None values in session_gateway_runtime's reader since gateway_runtime is replaced as a whole dict (not deep-merged), so None values written by the persist path survive in the nested dict. Found by /simplify-code 3-reviewer review on #85261 (all 3 reviewers converged on the route dict asymmetry as the verdict-relevant finding).
This commit is contained in:
parent
285eaaddc0
commit
9cb456a9b9
28
cli.py
28
cli.py
|
|
@ -7781,28 +7781,24 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin, CLIBillingMixin):
|
|||
) or None
|
||||
except Exception:
|
||||
provider = None
|
||||
# Both shapes use the same or-None discipline so stale keys from a
|
||||
# previous switch are deleted (not merely omitted) in BOTH the
|
||||
# nested gateway_runtime dict (CLI reader) and the top-level keys
|
||||
# (TUI gateway reader). _merge_model_config_json only deletes on
|
||||
# explicit None, so falsy values must be converted, not filtered.
|
||||
# Deriving the top-level from **route guarantees the two shapes
|
||||
# can never diverge — the asymmetry that caused the original
|
||||
# stale-key bug (#85261 simplify-code review).
|
||||
route = {
|
||||
k: v
|
||||
for k, v in {
|
||||
"provider": provider,
|
||||
"base_url": result.base_url,
|
||||
"api_mode": result.api_mode,
|
||||
}.items()
|
||||
if v
|
||||
"provider": provider or None,
|
||||
"base_url": result.base_url or None,
|
||||
"api_mode": result.api_mode or None,
|
||||
}
|
||||
try:
|
||||
db.update_session_model(sid, result.new_model)
|
||||
# Both shapes: nested for the CLI reader, top-level for the
|
||||
# TUI gateway's resume path. Top-level keys are written as
|
||||
# explicit None when absent — _merge_model_config_json only
|
||||
# deletes on None, so omitting them would let a PREVIOUS
|
||||
# switch's provider/api_mode survive this one (stale wire
|
||||
# protocol / frankenroute on resume).
|
||||
db.patch_session_model_config(sid, {
|
||||
"gateway_runtime": route,
|
||||
"provider": provider or None,
|
||||
"base_url": result.base_url or None,
|
||||
"api_mode": result.api_mode or None,
|
||||
**route,
|
||||
})
|
||||
except Exception:
|
||||
logger.debug(
|
||||
|
|
|
|||
|
|
@ -6132,7 +6132,10 @@ class SessionDB(SessionSearchMixin, SessionSchemaMixin, SessionPortabilityMixin)
|
|||
return {}
|
||||
runtime = raw.get("gateway_runtime")
|
||||
if isinstance(runtime, dict) and runtime.get("provider"):
|
||||
return dict(runtime)
|
||||
# Filter None values: the persist path writes or-None to trigger
|
||||
# deletion in the top-level merge, but gateway_runtime is replaced
|
||||
# as a whole dict (not deep-merged), so None values survive here.
|
||||
return {k: v for k, v in runtime.items() if v is not None}
|
||||
top_level = {
|
||||
key: raw.get(key)
|
||||
for key in ("provider", "base_url", "api_mode")
|
||||
|
|
|
|||
|
|
@ -147,9 +147,10 @@ def test_persist_model_switch_writes_model_and_both_route_shapes():
|
|||
# ...and top-level for the TUI gateway's _stored_session_runtime_overrides.
|
||||
assert patch["provider"] == "custom:opencode-zen"
|
||||
assert patch["base_url"] == "https://oz/v1"
|
||||
assert "api_mode" not in patch["gateway_runtime"] # empty values dropped
|
||||
# Absent top-level values are explicit None so the merge DELETES stale
|
||||
# keys from a previous switch (merge only deletes on None).
|
||||
# Both shapes use or-None so stale keys are deleted (not merely omitted)
|
||||
# in BOTH gateway_runtime and top-level — the asymmetry that caused the
|
||||
# original stale-key bug.
|
||||
assert patch["gateway_runtime"]["api_mode"] is None
|
||||
assert patch["api_mode"] is None
|
||||
|
||||
|
||||
|
|
@ -183,8 +184,16 @@ def test_persist_model_switch_clears_stale_route_keys(tmp_path, monkeypatch):
|
|||
|
||||
meta = db.get_session("stale1")
|
||||
config = json.loads(meta["model_config"])
|
||||
# Top-level keys: stale values deleted.
|
||||
assert config["provider"] == "openrouter"
|
||||
assert "api_mode" not in config, config # stale anthropic_messages deleted
|
||||
# Nested gateway_runtime: stale values replaced with None (the merge
|
||||
# replaces the entire gateway_runtime dict, not deep-merging its keys).
|
||||
# The reader's `or None` / `if v` filtering treats None the same as
|
||||
# absent, so stale values are effectively erased.
|
||||
gw = config.get("gateway_runtime", {})
|
||||
assert gw.get("provider") == "openrouter"
|
||||
assert gw.get("api_mode") is None # stale anthropic_messages erased
|
||||
runtime = SessionDB.session_gateway_runtime(meta)
|
||||
assert runtime["provider"] == "openrouter"
|
||||
assert "api_mode" not in runtime
|
||||
|
|
@ -235,7 +244,7 @@ def test_persist_model_switch_heals_bare_custom(monkeypatch):
|
|||
written.clear()
|
||||
stub._persist_model_switch_to_session(_BareResult())
|
||||
assert written["patch"]["provider"] is None
|
||||
assert "provider" not in written["patch"]["gateway_runtime"]
|
||||
assert written["patch"]["gateway_runtime"]["provider"] is None
|
||||
|
||||
|
||||
def test_restore_session_model_heals_bare_custom_stored_rows(monkeypatch):
|
||||
|
|
|
|||
Loading…
Reference in New Issue