fix(tui_gateway): reconcile every build-relevant override with the deferred agent build - #75385
Conversation
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the deferred-build window across the explicit-provider, MoA, fast-mode, and reasoning paths. The underlying race is present on current main: _start_agent_build snapshots overrides before _make_agent (tui_gateway/server.py:1960-1966) and installs the agent afterward (tui_gateway/server.py:1972).
Problems
- Blocking: the new reconciliation only applies reasoning when
create_reasoning_overrideis non-null (tui_gateway/server.py:1944-1947in this PR). But the existing global reasoning route writes the new global setting and removes that session key (tui_gateway/server.py:10382-10385), applying it live only when an agent already exists (tui_gateway/server.py:10393-10394). A globally scoped change during the parked build therefore leaves the agent on the old snapshotted reasoning value.
Suggested changes
- Treat an override removal as a reconciled transition and apply the effective global parsed setting after installation.
- Add the corresponding barrier regression test; the existing live-agent clear test is at
tests/test_tui_gateway_server.py:6214-6241.
This is an automated hermes-sweeper review.
| changed = True | ||
|
|
||
| reasoning = session.get("create_reasoning_override") | ||
| if reasoning is not None and reasoning != before.get("reasoning"): |
There was a problem hiding this comment.
Blocking: this ignores a cleared override. config.set reasoning with scope=global removes create_reasoning_override while the build can still be parked (server.py:10382-10385 on main); since the live apply also requires an agent, this leaves the newly installed agent with the old snapshotted reasoning config. Reconcile the removal to the new effective global value and add a barrier test for it.
There was a problem hiding this comment.
Good catch — fixed in ab86f7d1e.
You're right that a removal is a transition. The reconcile now handles the cleared case explicitly: when create_reasoning_override was present at snapshot time and is absent after installation, it resolves the effective global setting via parse_reasoning_effort(agent.reasoning_effort) from the freshly written config and applies that to the installed agent.
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:
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 = TrueBarrier test added as test_global_reasoning_clear_during_agent_build_reaches_installed_agent, driven through the real scope=global route (idiom borrowed from the live-agent clear test you pointed at). It asserts the pin is gone and the agent is still unbuilt at request time, then that the installed agent carries {"enabled": True, "effort": "high"}. Verified red before the change (assert None == {'effort': 'high', 'enabled': True}) and green after.
I also screened the other two snapshotted keys for the same removal shape: nothing pops create_service_tier_override or model_override during a deferred build. The only clearing site is _reset_session_agent (/new), which builds and installs its own agent synchronously rather than going through _start_agent_build, so it is not this race.
There was a problem hiding this comment.
Correction to the SHA above: ab86f7d1e was rebased away and resolves to nothing on this branch. The work is in 77438cd88c ("fix(tui_gateway): reconcile a cleared reasoning override with the deferred build"), which is on the current head b4e23402c9 — the commit after it did not touch this branch of the function. _reconcile_deferred_build_overrides now starts at tui_gateway/server.py:1948, with the reasoning handling at 2048–2068.
There a removal is treated as a transition: create_reasoning_override present in the pre-build snapshot and absent after installation falls to elif before_reasoning is not None, which re-reads the config through _load_cfg() — so it sees the agent.reasoning_effort that scope=global just wrote, not the snapshot — resolves it with parse_reasoning_effort, applies it to the installed agent, and sets changed so _persist_live_session_runtime runs. The barrier test is test_global_reasoning_clear_during_agent_build_reaches_installed_agent (tests/test_tui_gateway_server.py:15789): it seeds agent.reasoning_effort: medium in config.yaml, parks a build holding create_reasoning_override={"enabled": True, "effort": "low"}, then drives the real config.set route with key=reasoning, value=high, scope=global. It asserts the session pin is gone while session["agent"] is still None — i.e. the write genuinely landed inside the build window — and then, after releasing the build, that the installed agent carries {"enabled": True, "effort": "high"}. That value is distinct from both the snapshotted low and the seeded medium, so the test fails if the reconcile reads either instead of the new effective global.
There was a problem hiding this comment.
Correction to the SHA above: the branch has been rebased onto current main, so 77438cd88c and the head b4e23402c9 named above are no longer on it. The cleared-override work is now be46bbfa1bc ("fix(tui_gateway): reconcile a cleared reasoning override with the deferred build"), on the current head e5cce58bbab. Nothing about the fix changed — the rebase's only conflict was an append-at-EOF collision in tests/test_tui_gateway_server.py, and the branch diff against main is unchanged at 623 insertions / 3 deletions across the same three files.
The mechanism is as described: a removal is treated as a transition. create_reasoning_override present in the pre-build snapshot and absent after installation falls to elif before_reasoning is not None, which re-reads the config through _load_cfg() — so it sees the agent.reasoning_effort that scope=global just wrote rather than the snapshot — resolves it with parse_reasoning_effort, applies it to the installed agent, and sets changed so _persist_live_session_runtime runs.
Updated anchors: _reconcile_deferred_build_overrides starts at tui_gateway/server.py:2129, with the reasoning handling at 2228–2249. The barrier test keeps its name — test_global_reasoning_clear_during_agent_build_reaches_installed_agent, now tests/test_tui_gateway_server.py:16547. Full file passes: 530 passed.
There was a problem hiding this comment.
Correction to the SHA above: the branch has been rebased onto current main, so be46bbfa1bc and the head e5cce58bbab named in my previous note are no longer on it. The cleared-override work is now 655eae43769 ("fix(tui_gateway): reconcile a cleared reasoning override with the deferred build"), on the current head a13480d142f. Nothing about the fix changed — the branch diff against main is unchanged at 623 insertions / 3 deletions across the same three files, and the PR reads MERGEABLE again.
Since commit ids do not survive a rebase, the durable anchor for this thread is the test: test_global_reasoning_clear_during_agent_build_reaches_installed_agent in tests/test_tui_gateway_server.py. It parks a build inside _make_agent, issues config.set reasoning with scope=global — which writes agent.reasoning_effort and pops create_reasoning_override — then releases the build and asserts the installed agent carries the new effective global effort rather than the one the build snapshotted. The handling lives in _reconcile_deferred_build_overrides (tui_gateway/server.py, currently at :2129): a key present in the pre-build snapshot and absent after installation falls to the elif before_reasoning is not None branch, which re-reads the effective global value through parse_reasoning_effort and applies it to the agent. A removal is treated as a transition, as you asked.
One structural note on this rebase: main had grown its own block at the end of tests/test_tui_gateway_server.py, so rather than re-append behind it I moved the deferred-build tests up beside test_start_agent_build_passes_session_model_override, which is the cluster they belong with. No test bodies changed.
alt-glitch
added
type/bug
Related: #63998 addresses the same deferred-build override-loss race. This patch additionally covers explicit-provider model picks, /moa, fast tier, and failed-reconcile restoration; the two current mechanisms need a maintainer consolidation choice. |
teknium1
added
the
sweeper:blast-moderate
ab86f7d to
77438cd
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes a race in the TUI gateway’s deferred AIAgent build path where build-relevant session overrides (model, reasoning, fast/service-tier) can be written while _make_agent() is in-flight and then silently never reach the installed agent. The change adds a post-install reconciliation step and expands test coverage to exercise the real RPC writer routes during the build window.
Changes:
- Add
_reconcile_deferred_build_overrides()to adopt model / reasoning / fast-mode overrides written during deferred agent construction. - Update the
/moapre-agent branch comment to reflect that in-flight builds may have already snapshotted the previous override. - Add a suite of barrier-style tests covering each override writer path (including explicit-provider model switches,
/moa, global reasoning clears, and failed-switch rollback behavior).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tui_gateway/server.py |
Adds deferred-build override reconciliation and snapshots build-time override baselines. |
tui_gateway/methods_tools.py |
Updates /moa comment to match the new reconcile behavior during an in-flight build. |
tests/test_tui_gateway_server.py |
Adds regression tests that mutate overrides mid-build and assert the installed agent reflects them. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tier = session.get("create_service_tier_override") | ||
| if tier is not None and tier != before.get("tier"): | ||
| agent.service_tier = "priority" if tier == "priority" else None | ||
| request_overrides = dict(getattr(agent, "request_overrides", {}) or {}) | ||
| request_overrides.pop("service_tier", None) | ||
| request_overrides.pop("speed", 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: | ||
| request_overrides.update(fast_overrides) | ||
| agent.request_overrides = request_overrides | ||
| changed = True |
There was a problem hiding this comment.
Confirmed and fixed in b4e23402c.
The divergence is real. config.set fast resolves first and returns 4002 "fast mode is not available for this model" before it writes anything, so create_service_tier_override == "priority" has always carried the implication that resolve_fast_mode_overrides resolved for the session's model — agent.service_tier = "priority" is simply unreachable when it didn't. The reconcile applies the model switch first (deliberately — the switch rebuilds the client and fast overrides must resolve against the new model), which can invalidate that implication after the fact, and then set the tier unconditionally anyway. Result: a session advertising service_tier="priority" in session.info with no service_tier/speed in request_overrides — a state the live path structurally cannot produce.
Returning the live path's error isn't available here; nothing is listening to a background build, and the pin is already committed. So the fix mirrors the resolver's precedence rather than its error: when tier == "priority" and resolve_fast_mode_overrides() comes back None for the post-switch model, the pin is popped from the session and agent.service_tier is left None. The tier assignment is now "priority" if fast_overrides else None, which states the invariant the live path gets for free from its early return. Popping rather than pinning "" matters for rebuilds too — the user asked for fast, not for normal, so the session goes back to having no opinion instead of inheriting a stale unhonorable pin.
Regression test: test_reconcile_drops_the_fast_pin_when_the_switched_model_cannot_honor_it in tests/test_tui_gateway_server.py, beside the other build-window reconcile tests. It picks fast and then an unsupported model inside the same parked build window (fast first — picking the unsupported model first would just 4002 at the door), then asserts the installed agent has no tier, no service_tier/speed overrides, and no surviving pin. Verified to fail against the previous behaviour with assert 'priority' is None.
There was a problem hiding this comment.
Correcting a stale commit reference in my earlier reply on this thread — that SHA is no longer on the branch after a rebase, so the link points at nothing. Anchoring on the test name instead, which survives future rebases.
This is addressed on the current head. _reconcile_deferred_build_overrides now resolves the fast-mode overrides before deciding the tier: when resolve_fast_mode_overrides(agent.model) returns None for the post-switch model it pops create_service_tier_override from the session, and agent.service_tier is set to "priority" if fast_overrides else None. service_tier/speed are stripped from request_overrides unless the overrides actually resolved, so the session can no longer advertise service_tier="priority" in session.info with no matching request overrides behind it.
Erroring is not available on this path (nothing is listening to a background build), so it mirrors the resolver precedence the live path enforces and drops the pin instead.
Regression test: test_reconcile_drops_the_fast_pin_when_the_switched_model_cannot_honor_it in tests/test_tui_gateway_server.py.
| 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() | ||
| if model: | ||
| 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, | ||
| ) |
There was a problem hiding this comment.
Accepted — fixed in the same commit, b4e23402c.
The mechanism checks out. The guard is override != before.get("model"), and before is the pre-build snapshot rather than the agent's live state, so it cannot distinguish "picked during the build and never applied" from "already applied live". A writer taking its live branch in the window between current["agent"] = agent and this reconcile does both halves itself — mutates the agent and pins the rich model_override that this function then reads back — which leaves the guard true and re-runs the switch on a model the agent is already on. The duplicate isn't inert: _restart_slash_worker, a second _append_model_switch_marker entry in the transcript, and a redundant session.info emit.
I agree the window is narrow, so I kept the fix to the cheapest thing that closes it and reused the file's own idiom rather than inventing one — _sync_agent_model_with_config already makes exactly this baseline-adoption check for the same reason ("already running the configured model … adopt the baseline without a redundant switch"):
already_live = model == getattr(agent, "model", "") and (
not provider or provider == getattr(agent, "provider", "")
)
if model and not already_live:Worth noting why this can't under-apply: the guard only fires when the agent is already on the target, which by construction means a live-apply already ran and therefore already wrote the rich model_override (with base_url/api_key/api_mode). The other shape — a pre-build writer that pins without touching the agent — leaves agent.model on the built-in model, so the guard doesn't fire and the reconcile switches exactly as before.
Regression test: test_reconcile_skips_a_model_switch_a_live_writer_already_applied, which drives the reconcile with the post-live-apply state and asserts switch_model is never resolved, the client is never swapped, none of the three side effects fire, and the live writer's pin survives the skip. Verified to fail without the guard.
There was a problem hiding this comment.
Correcting a stale commit reference in my earlier reply on this thread — that SHA is no longer on the branch after a rebase, so the link points at nothing. Anchoring on the test name instead, which survives future rebases.
This is addressed on the current head. _reconcile_deferred_build_overrides now computes an already_live check before calling _apply_model_switch() — the agent's model matches the requested override, and its provider matches when one is set — and skips the switch in that case. It is the same baseline-adoption check _sync_agent_model_with_config makes.
That closes exactly the race described here: a writer that took its LIVE branch between the agent's installation and this call has already applied the switch and pinned the model_override we read, but before is the pre-build snapshot so the earlier guard could not see it. The redundant call no longer produces a second slash-worker restart, a duplicate switch marker in the transcript, or a redundant session.info.
Regression test: test_reconcile_skips_a_model_switch_a_live_writer_already_applied in tests/test_tui_gateway_server.py.
b4e2340 to
e5cce58
Compare
…ferred agent build _start_agent_build snapshots model_override, create_reasoning_override and create_service_tier_override into the build kwargs, then blocks for seconds inside _make_agent (MCP discovery, prompt/skill build) before installing the agent. Every writer of those three keys gates its live-apply on session["agent"] being set, so a pick made inside that window reaches neither the kwargs (already read) nor the agent (not yet installed). The user's choice is silently lost for the life of the session. The racy writers are 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, so it is the easiest to hit. Snapshot the three keys immediately before _make_agent and reconcile them once the agent is installed and wired, applying whatever changed through the same helpers the live paths use. Model is reconciled first: the switch rebuilds the agent's client and fast-mode overrides must resolve against the new model. A failed reconcile restores the previously built override instead of retaining the failed target. This matches the live path, which raises before committing model_override (NousResearch#50163); a retained failure would otherwise be resurrected by the next /new or resume and rebuild the session onto a model that does not work.
…erred build config.set reasoning with scope=global writes agent.reasoning_effort and pops create_reasoning_override, applying the new effort live only when an agent already exists. Both halves are skipped during the deferred build window, so the reconcile treated the removal as "nothing changed" and the freshly installed agent kept the effort the build had snapshotted. Treat an override removal as a reconciled transition and apply the effective global parsed setting after installation, with a barrier regression test through the real global-scope route.
…esolver it mirrors
Two divergences in _reconcile_deferred_build_overrides, both stemming
from it comparing against the pre-build snapshot rather than the agent's
live state.
Fast tier: the tier branch set agent.service_tier unconditionally and
only applied resolve_fast_mode_overrides when it returned something. The
live config.set fast path does the opposite — it resolves first and
returns 4002 ("fast mode is not available for this model") before
pinning anything, so create_service_tier_override == "priority" has
always implied the overrides resolved. The model switch this function
reconciles first can land on a model without fast support, at which
point the session advertised service_tier="priority" in session.info
with no request overrides behind it. The reconcile cannot error (nothing
is listening to a background build), so mirror the resolver's precedence
instead: drop the pin and leave the tier unset. The docstring already
said fast-mode overrides must resolve against the newly selected model;
the code did not finish the thought.
Model switch: the guard is `override != before.get("model")`, and before
is the pre-build snapshot, not live state. A writer taking its live
branch after session["agent"] is set but before this runs applies the
switch itself and pins the same model_override read back here, leaving
the guard true and re-running the switch — a second slash-worker
restart, a second switch marker in the transcript, a redundant
session.info. Adopt the state instead when the agent already runs the
requested model and provider, the same baseline check
_sync_agent_model_with_config makes.
e5cce58 to
a13480d
Compare
Supersedes #63998.
What does this PR do?
_start_agent_buildsnapshotsmodel_override,create_reasoning_overrideandcreate_service_tier_overrideinto the build kwargs (tui_gateway/server.py),then calls
_make_agent, which blocks for seconds (MCP discovery, prompt/skillbuild), and finally installs the result with
current["agent"] = agent— withno re-read of those keys.
Every writer of those keys gates its live-apply on
session["agent"]being set.So a pick made during the build window reaches neither the kwargs (already
snapshotted) nor the agent (not yet installed), and is silently lost for the
life of the session. The user picks a model, sees no error, and the session runs
on the old one until they restart it.
#63998 identified this race but locks
reasoningonly. The maintainer reviewon that PR asked for two things it does not do — this PR implements both:
restore/clear a failed reconciled model override.
/moaroutes, plus a failed-switch regression test.
That PR also retains a failed
switch_model()target inmodel_override; thisone restores it, matching the live path. (#63998 additionally predates
f67ca220a refactor(tui): split @method handlers into methods_* modules, so itsdiff is written against the pre-refactor layout and it currently reads
CONFLICTING.)Related Issue
No filed issue — found via the deferred-build path in
tui_gateway/server.py.Type of Change
Changes Made
tui_gateway/server.py— new_reconcile_deferred_build_overrides(); snapshotthe three keys immediately before
_make_agent; reconcile after_wire_callbacks, so the existingsession.infoemit publishes the reconciledidentity. Model is reconciled first (the switch rebuilds the client, and
fast-mode overrides resolve against the new model), then tier, then reasoning
(a
switch_modelcan resetreasoning_config).tui_gateway/methods_tools.py— correct the stale/moacomment.Writer coverage (the whole root cause, not one key)
config.set reasoning(session scope)create_reasoning_overrideconfig.set reasoning(global scope)create_reasoning_overrideconfig.set modelw/ explicit providermodel_override_wait_agentinitialization wait entirely_apply_model_switchmodel_override/moamodel_overrideconfig.set fastcreate_service_tier_overridemethods_session.pyresumemodel_overrideThe
/moaelse-branch carried the comment "the override is consumed by thefirst build" — false while a build is already in flight, and the site's own
neighbouring comment already records the sibling failure (
#53444: setting session["model_override"] alone never switched the already-built agent). Thecomment is now accurate.
Failed-switch handling
A failed reconcile restores the override the build baked in (or clears it)
instead of retaining the failed target. This matches the live path, which raises
before committing
model_override(#50163). A retained failure would beresurrected by the next
/newor resume, rebuilding onto a model that does notwork.
The reconcile passes
confirm_expensive_model=True: the pick is alreadycommitted to the session, so its cost confirmation was answered by whoever wrote
it, and nothing is listening to this background build — a
confirm_requiredreturn would drop the user's model on the floor.
How to Test
uv run --with pytest --with pytest-asyncio python3 -m pytest tests/test_tui_gateway_server.py -q→ 501 passed._make_agent(mirroring theexisting
_slow_make_agentbarrier idiom in this file), issue the overridethrough its real RPC route while
session["agent"]is stillNone,release the build, then assert the installed agent carries the pick.
Red-before proof (two-way)
test_reasoning_pick_during_agent_build_reaches_installed_agenttest_global_reasoning_clear_during_agent_build_reaches_installed_agenttest_explicit_provider_model_switch_during_agent_build_reaches_agenttest_moa_one_shot_during_agent_build_reaches_agenttest_failed_reconcile_does_not_leave_failed_model_override_pinnedtest_fast_mode_pick_during_agent_build_reaches_installed_agentThe middle column is the point: reducing this PR to #63998's coverage leaves five
of the six failures live.
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/ARelated / Positioning
site but fixes a different defect — a session dying mid-build leaving an
unclosed agent. No overlap with override staleness; both can land.
run_after_agent_ready) and fix(agent): preserve session reasoning_config when switch_model resolves to None #72857 (switch_modelresetting reasoningat the agent layer) are disjoint.
reasoningonly, and it is
CONFLICTINGagainst current main after themethods_*refactor. This PR is the superset (see the writer-coverage and red-before
tables above), so consolidating onto it loses nothing.