diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index 2df2b124e37a4..8763846bb0287 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -15022,6 +15022,480 @@ def fake_make_agent(sid, key, session_id=None, session_db=None, **kwargs): server._sessions.clear() +# --------------------------------------------------------------------------- +# Deferred agent build <-> override race. +# +# _start_agent_build snapshots model_override / create_reasoning_override / +# create_service_tier_override into the build kwargs and then blocks for +# seconds inside _make_agent (MCP discovery, prompt/skill build). Every writer +# of those keys gates its live-apply on session["agent"] being set, so a pick +# made inside that window reached neither the kwargs (already read) nor the +# agent (not yet installed) and was silently lost for the life of the session. +# --------------------------------------------------------------------------- + + +class _BuildRaceAgent: + """Stand-in for the agent _make_agent returns, recording in-place switches.""" + + def __init__(self): + self.model = "old/model" + self.provider = "openrouter" + self.base_url = "" + self.api_key = "sk-or" + self.api_mode = "chat_completions" + self.reasoning_config = None + self.service_tier = None + self.request_overrides: dict = {} + self.tools: list = [] + self.switch_calls: list = [] + + def switch_model(self, **kwargs): + self.switch_calls.append(kwargs) + self.model = kwargs.get("new_model", self.model) + self.provider = kwargs.get("new_provider", self.provider) + + +def _park_agent_build(monkeypatch, **session_extra): + """Start a deferred agent build parked inside _make_agent. + + Returns ``(sid, session, agent, release)``. Until ``release`` is set the + build thread sits inside _make_agent with ``session["agent"]`` still None — + the exact window every override writer used to mishandle. + """ + agent = _BuildRaceAgent() + entered = threading.Event() + release = threading.Event() + + def _slow_make_agent(_sid, _key, **_kwargs): + entered.set() + release.wait(timeout=5.0) + return agent + + monkeypatch.setattr(server, "_make_agent", _slow_make_agent) + monkeypatch.setattr(server, "_wire_callbacks", lambda _sid: None) + monkeypatch.setattr(server, "_emit", lambda *a, **kw: None) + monkeypatch.setattr(server, "_session_info", lambda _a, *a2: {"model": "x"}) + monkeypatch.setattr(server, "_probe_config_health", lambda _cfg: None) + monkeypatch.setattr(server, "_config_model_target", lambda: ("old/model", "")) + monkeypatch.setattr(server, "_start_notification_poller", lambda _sid, _s: None) + monkeypatch.setattr(server, "_notify_session_boundary", lambda *a, **kw: None) + monkeypatch.setattr(server, "_schedule_mcp_late_refresh", lambda *a, **kw: None) + monkeypatch.setattr(server, "_persist_live_session_runtime", lambda _s: None) + monkeypatch.setattr(server, "_persist_live_session_system_prompt", lambda _s: None) + monkeypatch.setattr(server, "_restart_slash_worker", lambda _sid, _s: None) + monkeypatch.setattr(server, "_append_model_switch_marker", lambda *a, **kw: None) + monkeypatch.setattr( + server, + "_get_db", + lambda: types.SimpleNamespace(create_session=lambda *a, **kw: None), + ) + + import tools.approval as _approval + + monkeypatch.setattr(_approval, "register_gateway_notify", lambda key, cb: None) + monkeypatch.setattr(_approval, "unregister_gateway_notify", lambda key: None) + monkeypatch.setattr(_approval, "load_permanent_allowlist", lambda: None) + + sid = "build-race-sid" + session = _session(agent_ready=threading.Event(), **session_extra) + session["agent"] = None + server._sessions[sid] = session + server._start_agent_build(sid, session) + assert entered.wait(timeout=2.0), "build thread never entered _make_agent" + assert session.get("agent") is None, "agent must not be installed yet" + return sid, session, agent, release + + +def _finish_build(session, release): + release.set() + assert session["agent_ready"].wait(timeout=5.0), "build never completed" + + +def _switch_result(model, provider, **extra): + return types.SimpleNamespace( + success=True, + new_model=model, + target_provider=provider, + api_key=extra.get("api_key", "sk-test"), + base_url=extra.get("base_url", "https://example.invalid"), + api_mode=extra.get("api_mode", "chat_completions"), + warning_message="", + model_info=None, + ) + + +def test_reasoning_pick_during_agent_build_reaches_installed_agent(monkeypatch): + """#63998's original shape: /reasoning during the build window was dropped. + + config.set reasoning writes create_reasoning_override and then gates the + live apply on session["agent"] — None mid-build — so the effort never + reached the agent the build was about to install. + """ + sid, session, agent, release = _park_agent_build(monkeypatch) + try: + resp = server.handle_request( + { + "id": "1", + "method": "config.set", + "params": {"session_id": sid, "key": "reasoning", "value": "high"}, + } + ) + assert resp.get("result"), f"got error: {resp.get('error')}" + assert session.get("agent") is None, "build must still be in flight" + + _finish_build(session, release) + + assert session["create_reasoning_override"] is not None + assert agent.reasoning_config == session["create_reasoning_override"], ( + "reasoning picked during the build window never reached the agent" + ) + finally: + release.set() + server._sessions.pop(sid, None) + + +def test_global_reasoning_clear_during_agent_build_reaches_installed_agent( + tmp_path, monkeypatch +): + """A *cleared* override is a reconciled transition too. + + config.set reasoning with scope=global writes agent.reasoning_effort and + pops create_reasoning_override, applying it live only when an agent already + exists. During the build window neither happens, so the installed agent + would otherwise keep the effort the build snapshotted. + """ + monkeypatch.setattr(server, "_hermes_home", tmp_path) + (tmp_path / "config.yaml").write_text( + "agent:\n reasoning_effort: medium\n", encoding="utf-8" + ) + + sid, session, agent, release = _park_agent_build( + monkeypatch, + create_reasoning_override={"enabled": True, "effort": "low"}, + ) + try: + resp = server.handle_request( + { + "id": "1", + "method": "config.set", + "params": { + "session_id": sid, + "key": "reasoning", + "value": "high", + "scope": "global", + }, + } + ) + assert resp.get("result"), f"got error: {resp.get('error')}" + # The session pin is gone and the agent does not exist yet. + assert "create_reasoning_override" not in session + assert session.get("agent") is None, "build must still be in flight" + + _finish_build(session, release) + + assert agent.reasoning_config == {"enabled": True, "effort": "high"}, ( + "a global reasoning change during the build window left the agent " + "on the stale snapshotted effort" + ) + finally: + release.set() + server._sessions.pop(sid, None) + + +def test_explicit_provider_model_switch_during_agent_build_reaches_agent(monkeypatch): + """config.set model with an explicit provider skips the initialization wait. + + That route calls _apply_model_switch with session["agent"] still None, so it + only records model_override; without reconciliation the built agent keeps + running the model the build snapshotted. + """ + result = _switch_result("claude-sonnet-4.6", "anthropic") + monkeypatch.setattr( + "hermes_cli.model_switch.switch_model", lambda **_kwargs: result + ) + + sid, session, agent, release = _park_agent_build(monkeypatch) + try: + resp = server.handle_request( + { + "id": "1", + "method": "config.set", + "params": { + "session_id": sid, + "key": "model", + "value": "claude-sonnet-4.6 --provider anthropic", + "confirm_expensive_model": True, + }, + } + ) + assert resp.get("result"), f"got error: {resp.get('error')}" + # The explicit-provider path returned without ever waiting for a build. + assert session.get("agent") is None, "build must still be in flight" + assert session["model_override"]["model"] == "claude-sonnet-4.6" + + _finish_build(session, release) + + assert agent.switch_calls, ( + "model picked during the build window never reached the agent" + ) + assert agent.model == "claude-sonnet-4.6" + assert agent.provider == "anthropic" + finally: + release.set() + server._sessions.pop(sid, None) + + +def test_moa_one_shot_during_agent_build_reaches_agent(monkeypatch): + """The pre-agent /moa branch writes model_override and returns. + + Its comment claimed "the override is consumed by the first build", which is + false once the build is already in flight — the MoA turn silently ran on the + old model. + """ + monkeypatch.setattr( + "hermes_cli.moa_config.normalize_moa_config", + lambda _cfg: {"default_preset": "balanced"}, + ) + result = _switch_result("balanced", "moa", base_url="moa://local") + monkeypatch.setattr( + "hermes_cli.model_switch.switch_model", lambda **_kwargs: result + ) + + sid, session, agent, release = _park_agent_build(monkeypatch) + try: + resp = server.handle_request( + { + "id": "1", + "method": "command.dispatch", + "params": {"session_id": sid, "name": "moa", "arg": "summarize this"}, + } + ) + assert resp.get("result"), f"got error: {resp.get('error')}" + assert session["model_override"]["provider"] == "moa" + + _finish_build(session, release) + + assert agent.switch_calls, ( + "/moa preset picked during the build window never reached the agent" + ) + assert agent.provider == "moa" + assert agent.model == "balanced" + finally: + release.set() + server._sessions.pop(sid, None) + + +def test_failed_reconcile_does_not_leave_failed_model_override_pinned(monkeypatch): + """A failed reconcile must not persist the model it failed to switch to. + + The live switch path raises *before* committing model_override (#50163). + The reconcile must match it: a retained failed target would be resurrected + by the next /new or resume and rebuild the session onto a broken model. + """ + result = _switch_result("broken/model", "anthropic") + monkeypatch.setattr( + "hermes_cli.model_switch.switch_model", lambda **_kwargs: result + ) + + sid, session, agent, release = _park_agent_build(monkeypatch) + try: + resp = server.handle_request( + { + "id": "1", + "method": "config.set", + "params": { + "session_id": sid, + "key": "model", + "value": "broken/model --provider anthropic", + "confirm_expensive_model": True, + }, + } + ) + assert resp.get("result"), f"got error: {resp.get('error')}" + assert session["model_override"]["model"] == "broken/model" + + # The in-place swap fails only once the build thread reconciles. + def _boom(**_kwargs): + raise RuntimeError("provider rejected the model") + + agent.switch_model = _boom + + _finish_build(session, release) + + assert session.get("model_override") is None, ( + "a failed reconcile left the failed model pinned on the session; " + "the next /new or resume would rebuild onto it" + ) + finally: + release.set() + server._sessions.pop(sid, None) + + +def test_fast_mode_pick_during_agent_build_reaches_installed_agent(monkeypatch): + """create_service_tier_override is the third key the build snapshots. + + config.set fast pins it, then gates the live apply on the agent existing — + the same race as reasoning. + """ + monkeypatch.setattr( + "hermes_cli.models.resolve_fast_mode_overrides", + lambda _model: {"service_tier": "priority"}, + ) + + sid, session, agent, release = _park_agent_build(monkeypatch) + try: + resp = server.handle_request( + { + "id": "1", + "method": "config.set", + "params": {"session_id": sid, "key": "fast", "value": "fast"}, + } + ) + assert resp.get("result"), f"got error: {resp.get('error')}" + assert session.get("agent") is None, "build must still be in flight" + assert session["create_service_tier_override"] == "priority" + + _finish_build(session, release) + + assert agent.service_tier == "priority", ( + "fast mode picked during the build window never reached the agent" + ) + finally: + release.set() + server._sessions.pop(sid, None) + + +def test_reconcile_drops_the_fast_pin_when_the_switched_model_cannot_honor_it( + monkeypatch, +): + """Reconciling both keys must not break the resolver's own invariant. + + ``config.set fast`` returns 4002 ("fast mode is not available for this + model") *before* pinning anything, so ``create_service_tier_override == + "priority"`` has always implied that ``resolve_fast_mode_overrides`` + resolved for the session's model. A model picked in the same build window + is reconciled first and can land on a model without fast support, at which + point setting the tier unconditionally leaves the session advertising + ``service_tier="priority"`` with no request overrides behind it — a state + the live path cannot produce. + """ + monkeypatch.setattr(server, "_resolve_model", lambda: "old/model") + monkeypatch.setattr( + "hermes_cli.models.resolve_fast_mode_overrides", + lambda model: None if model == "no-fast/model" else {"service_tier": "priority"}, + ) + monkeypatch.setattr( + "hermes_cli.model_switch.switch_model", + lambda **_kwargs: _switch_result("no-fast/model", "anthropic"), + ) + + sid, session, agent, release = _park_agent_build(monkeypatch) + try: + # Fast first: it validates against the model the session has *now*, so + # picking the unsupported model first would simply 4002 here. + resp = server.handle_request( + { + "id": "1", + "method": "config.set", + "params": {"session_id": sid, "key": "fast", "value": "fast"}, + } + ) + assert resp.get("result"), f"got error: {resp.get('error')}" + assert session["create_service_tier_override"] == "priority" + + resp = server.handle_request( + { + "id": "2", + "method": "config.set", + "params": { + "session_id": sid, + "key": "model", + "value": "no-fast/model --provider anthropic", + "confirm_expensive_model": True, + }, + } + ) + assert resp.get("result"), f"got error: {resp.get('error')}" + assert session.get("agent") is None, "build must still be in flight" + + _finish_build(session, release) + + assert agent.model == "no-fast/model", "the model reconcile must run first" + assert agent.service_tier is None, ( + "the session advertises priority service tier on a model whose fast " + "overrides do not resolve; config.set fast refuses to create that state" + ) + assert "service_tier" not in agent.request_overrides + assert "speed" not in agent.request_overrides + assert session.get("create_service_tier_override") is None, ( + "an unhonorable fast pin must not survive to the next rebuild" + ) + finally: + release.set() + server._sessions.pop(sid, None) + + +def test_reconcile_skips_a_model_switch_a_live_writer_already_applied(monkeypatch): + """``before`` is the pre-build snapshot, not the agent's live state. + + A writer that takes its live branch after ``session["agent"]`` is set but + before reconciliation runs applies the switch itself and pins the same + ``model_override`` this function then reads, so the ``!= before`` guard + stays true and the switch runs a second time. The duplicate is not + harmless: it restarts the slash worker, appends a second switch marker to + the transcript, and re-emits ``session.info``. + """ + switch_calls: list = [] + monkeypatch.setattr( + "hermes_cli.model_switch.switch_model", + lambda **kwargs: switch_calls.append(kwargs) + or _switch_result("new/model", "anthropic"), + ) + + side_effects: list[str] = [] + monkeypatch.setattr( + server, "_restart_slash_worker", lambda *_a: side_effects.append("slash_worker") + ) + monkeypatch.setattr( + server, + "_append_model_switch_marker", + lambda *_a, **_kw: side_effects.append("switch_marker"), + ) + monkeypatch.setattr( + server, "_emit", lambda kind, *_a, **_kw: side_effects.append(f"emit:{kind}") + ) + monkeypatch.setattr(server, "_session_info", lambda _a, *_a2: {"model": "x"}) + monkeypatch.setattr(server, "_persist_live_session_runtime", lambda _s: None) + monkeypatch.setattr(server, "_persist_live_session_system_prompt", lambda _s: None) + + agent = _BuildRaceAgent() + # The concurrent live-apply already landed: the agent runs the target model + # and the rich override it pinned is what the reconcile reads back. + agent.model = "new/model" + agent.provider = "anthropic" + session = _session(agent=agent) + session["model_override"] = { + "model": "new/model", + "provider": "anthropic", + "base_url": "https://example.invalid", + "api_key": "sk-test", + "api_mode": "chat_completions", + } + + server._reconcile_deferred_build_overrides( + "sid", session, agent, {"model": None, "reasoning": None, "tier": None} + ) + + assert switch_calls == [], "resolved a switch for a model already installed" + assert agent.switch_calls == [], "swapped the client for a no-op switch" + assert side_effects == [], ( + "duplicated the live writer's side effects: " f"{side_effects}" + ) + assert session["model_override"]["model"] == "new/model", ( + "the pin the live writer wrote must survive the skip" + ) + + # ── billing/subscription state + error serialization ───────────────── diff --git a/tui_gateway/methods_tools.py b/tui_gateway/methods_tools.py index dc12130c6fd30..8efcbf11fdc85 100644 --- a/tui_gateway/methods_tools.py +++ b/tui_gateway/methods_tools.py @@ -635,9 +635,12 @@ def _(rid, params: dict) -> dict: session.pop("moa_one_shot_restore", None) return _err(rid, 5030, f"moa unavailable: {exc}") else: - # No agent built yet (lazy/fresh session): the override is - # consumed by the first build, so the turn runs MoA without an - # in-place switch. + # No agent built yet (lazy/fresh session): the first build + # consumes the override, so the turn runs MoA without an + # in-place switch. If a build is already in flight it has + # already snapshotted the previous value, so + # _reconcile_deferred_build_overrides applies this one to the + # agent once it is installed. session["model_override"] = { "provider": "moa", "model": preset, diff --git a/tui_gateway/server.py b/tui_gateway/server.py index c33cba50ea977..3490dca6d8b9e 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -2126,6 +2126,132 @@ def _wait_agent_for_prompt(session: dict, rid: str, sid: str) -> dict | None: return _err(rid, 5032, err) if err else None +def _reconcile_deferred_build_overrides( + sid: str, session: dict, agent, before: dict +) -> None: + """Apply build-relevant overrides written while the agent was being built. + + ``_start_agent_build`` reads ``model_override`` / + ``create_reasoning_override`` / ``create_service_tier_override`` into the + build kwargs and then spends seconds inside ``_make_agent`` (MCP discovery, + prompt/skill build). Every writer of those keys gates its live-apply on + ``session["agent"]`` being set — ``config.set reasoning``, + ``config.set fast``, ``_apply_model_switch``, the pre-agent ``/moa`` branch, + and the explicit-provider ``config.set model`` path (which skips the + initialization wait entirely). A pick made inside that window therefore + reaches neither the kwargs (already snapshotted) nor the agent (not yet + installed), so it is silently lost for the life of the session. + + Re-read the three keys after installation and apply whatever changed + through the same helpers the live paths use. Model goes first: the switch + rebuilds the agent's client, and fast-mode overrides must resolve against + the newly selected model. + """ + override = session.get("model_override") + if isinstance(override, dict) and override != before.get("model"): + model = str(override.get("model") or "").strip() + provider = str(override.get("provider") or "").strip() + # Already running the requested model: a writer that took its LIVE + # branch between the agent's installation and this call has applied the + # switch itself and pinned the ``model_override`` we just read, but + # ``before`` is the pre-build snapshot, so the guard above cannot see + # that. Re-running the switch would duplicate its side effects — a + # second slash-worker restart, a second switch marker in the + # transcript, a redundant ``session.info`` — for no change. Same + # baseline-adoption check ``_sync_agent_model_with_config`` makes. + already_live = model == getattr(agent, "model", "") and ( + not provider or provider == getattr(agent, "provider", "") + ) + if model and not already_live: + raw = f"{model} --provider {provider}" if provider else model + try: + _apply_model_switch( + sid, + session, + raw, + # The pick is already committed to the session, so its + # cost confirmation (if any) was answered by whoever wrote + # it. Re-prompting here would strand the switch: nothing is + # listening to this background build, so a confirm_required + # return would drop the user's model on the floor. + confirm_expensive_model=True, + pin_session_override=True, + # Reconciling a session-scoped pick — never write config.yaml. + persist_override=False, + ) + except Exception as exc: + # The live path raises *before* committing the override + # (#50163), so a failed reconcile must not leave the failed + # target pinned: a later /new or resume rebuilds from + # model_override and would resurrect a model that does not + # work. Put back exactly what the build baked in. + logger.warning( + "Deferred-build model override reconcile failed: %s", exc + ) + prior = before.get("model") + if prior is None: + session.pop("model_override", None) + else: + session["model_override"] = prior + + changed = False + + tier = session.get("create_service_tier_override") + if tier is not None and tier != before.get("tier"): + fast_overrides = None + if tier == "priority": + from hermes_cli.models import resolve_fast_mode_overrides + + fast_overrides = resolve_fast_mode_overrides(getattr(agent, "model", None)) + if fast_overrides is None: + # ``config.set fast`` refuses the pin outright (4002, "fast mode + # is not available for this model") rather than recording a tier + # the model cannot honor, so ``create_service_tier_override == + # "priority"`` has always implied resolvable overrides. The + # model switch reconciled above can invalidate that after the + # fact. Erroring is not available here — nothing is listening + # to a background build — so mirror the resolver precedence the + # live path enforces and drop the pin instead of advertising + # ``service_tier="priority"`` in ``session.info`` with no + # matching request overrides behind it. + session.pop("create_service_tier_override", None) + # Priority iff the overrides actually resolved: the same invariant the + # live path gets from its early return. + agent.service_tier = "priority" if fast_overrides else None + request_overrides = dict(getattr(agent, "request_overrides", {}) or {}) + request_overrides.pop("service_tier", None) + request_overrides.pop("speed", None) + if fast_overrides: + request_overrides.update(fast_overrides) + agent.request_overrides = request_overrides + changed = True + + reasoning = session.get("create_reasoning_override") + before_reasoning = before.get("reasoning") + if reasoning is not None: + if reasoning != before_reasoning: + agent.reasoning_config = reasoning + changed = True + elif before_reasoning is not None: + # The session pin was *cleared* during the build: `config.set reasoning` + # with scope=global writes agent.reasoning_effort and pops the session + # key, then applies it live only when an agent already exists. A + # removal is just as much a reconciled transition as a write — without + # this the freshly installed agent keeps the stale snapshotted effort + # even though the user moved the setting globally. + from hermes_constants import parse_reasoning_effort + + cfg = _load_cfg() + agent_cfg = cfg.get("agent") if isinstance(cfg.get("agent"), dict) else {} + global_reasoning = parse_reasoning_effort(agent_cfg.get("reasoning_effort")) + if global_reasoning is not None: + agent.reasoning_config = global_reasoning + changed = True + + if changed: + _persist_live_session_runtime(session) + + def _start_agent_build(sid: str, session: dict) -> None: """Start building the real AIAgent for a TUI session, once. @@ -2229,6 +2355,17 @@ def _build() -> None: kw["reasoning_config_override"] = reasoning if (tier := current.get("create_service_tier_override")) is not None: kw["service_tier_override"] = tier + # Snapshot what the build is about to bake in. _make_agent + # blocks for seconds and every writer of these keys skips its + # live-apply while session["agent"] is None, so anything the + # user picks from here until installation must be reconciled + # afterwards or it is lost — see + # _reconcile_deferred_build_overrides. + pre_build_overrides = { + "model": current.get("model_override"), + "reasoning": current.get("create_reasoning_override"), + "tier": current.get("create_service_tier_override"), + } agent = _make_agent(sid, key, **kw) finally: _clear_session_context(tokens) @@ -2265,6 +2402,12 @@ def _build() -> None: pass _wire_callbacks(sid) + # The agent is installed and wired, so writers now take their live + # branch. Adopt anything picked during the build window before the + # session.info below publishes the session's runtime identity. + _reconcile_deferred_build_overrides( + sid, current, agent, pre_build_overrides + ) # Surface the self-improvement review's "💾 …" summary as an event # the TUI/desktop render in-transcript, honoring # display.memory_notifications. _init_session wires this for the