Merge pull request #85429 from NousResearch/jb/yolo-settings-session-info-emit
fix(desktop): re-emit session.info when approvals config changes out of band
This commit is contained in:
jonny
@@ -7595,6 +7595,7 @@ def _denormalize_config_from_web(config: Dict[str, Any]) -> Dict[str, Any]:
|
||||
@app.put("/api/config")
|
||||
async def update_config(body: ConfigUpdate, profile: Optional[str] = None):
|
||||
def _run():
|
||||
approvals_mode_changed = False
|
||||
with _profile_scope(body.profile or profile):
|
||||
# The dashboard form is schema-driven (see CONFIG_SCHEMA). Any root
|
||||
# key absent from the schema — most visibly ``custom_providers``, but
|
||||
@@ -7605,7 +7606,23 @@ async def update_config(body: ConfigUpdate, profile: Optional[str] = None):
|
||||
with _CONFIG_MUTATION_LOCK:
|
||||
existing = read_raw_config()
|
||||
incoming = _denormalize_config_from_web(body.config)
|
||||
save_config(_deep_merge(existing, incoming))
|
||||
merged = _deep_merge(existing, incoming)
|
||||
# Compare normalized approvals.mode across the in-memory
|
||||
# documents, not config blocks and not cache re-reads: the
|
||||
# settings page PUTs the defaulted GET record while disk
|
||||
# holds sparse YAML, so a block compare is always-unequal
|
||||
# (every autosave would broadcast), and reloading after the
|
||||
# save can serve the pre-save cache on an (mtime_ns, size)
|
||||
# key collision. Only approvals.mode feeds session.info, so
|
||||
# it is the honest trigger.
|
||||
approvals_mode_changed = _approval_mode_of(merged) != _approval_mode_of(existing)
|
||||
save_config(merged)
|
||||
# REST saves bypass the config.set RPC (which re-emits itself), so
|
||||
# refresh live sessions' cached approval/YOLO indicators after a mode
|
||||
# change. Own-profile saves only: a profile-scoped save targets a
|
||||
# different HERMES_HOME than this process's gateway sessions.
|
||||
if approvals_mode_changed and not _is_other_profile(body.profile or profile):
|
||||
_broadcast_gateway_session_info()
|
||||
return {"ok": True}
|
||||
|
||||
try:
|
||||
@@ -7617,6 +7634,51 @@ async def update_config(body: ConfigUpdate, profile: Optional[str] = None):
|
||||
raise HTTPException(status_code=500, detail="Internal server error")
|
||||
|
||||
|
||||
def _is_other_profile(profile: Optional[str]) -> bool:
|
||||
"""True when ``profile`` names a profile other than this process's own."""
|
||||
requested = (profile or "").strip()
|
||||
if not requested or requested.lower() == "current":
|
||||
return False
|
||||
try:
|
||||
target = _resolve_profile_dir(requested)
|
||||
except HTTPException:
|
||||
return True
|
||||
return target.resolve() != get_process_hermes_home().resolve()
|
||||
|
||||
|
||||
def _approval_mode_of(config: Dict[str, Any]) -> str:
|
||||
"""Normalize approvals.mode from an in-memory config document.
|
||||
|
||||
Both sides of the broadcast comparison use in-memory documents (the raw
|
||||
on-disk dict and the about-to-be-saved dict): re-reading through the
|
||||
config cache after a save can serve the pre-save document when the
|
||||
replacement file collides on the (mtime_ns, size) cache key, which would
|
||||
suppress the broadcast exactly when the mode changed. Absent block or
|
||||
key normalizes to the same default the approval gate uses.
|
||||
"""
|
||||
from tools.approval import _normalize_approval_mode
|
||||
|
||||
approvals = config.get("approvals")
|
||||
default_mode = (DEFAULT_CONFIG.get("approvals") or {}).get("mode", "manual")
|
||||
mode = approvals.get("mode", default_mode) if isinstance(approvals, dict) else default_mode
|
||||
return _normalize_approval_mode(mode)
|
||||
|
||||
|
||||
def _broadcast_gateway_session_info() -> None:
|
||||
"""Broadcast session.info on the in-process gateway when it's loaded.
|
||||
|
||||
``sys.modules`` guard, not an import: gateway never imported means no
|
||||
live sessions in this process to notify.
|
||||
"""
|
||||
server = sys.modules.get("tui_gateway.server")
|
||||
if server is None:
|
||||
return
|
||||
try:
|
||||
server.broadcast_session_info()
|
||||
except Exception:
|
||||
_log.exception("session.info broadcast after config save failed")
|
||||
|
||||
|
||||
def _catalog_provider_env_metadata() -> dict:
|
||||
"""Map provider env vars → desktop card metadata, derived from the catalog.
|
||||
|
||||
@@ -15160,10 +15222,15 @@ async def update_config_raw(body: RawConfigUpdate, profile: Optional[str] = None
|
||||
parsed = yaml.safe_load(body.yaml_text)
|
||||
if not isinstance(parsed, dict):
|
||||
raise HTTPException(status_code=400, detail="YAML must be a mapping")
|
||||
approvals_mode_changed = False
|
||||
with _profile_scope(body.profile or profile):
|
||||
# Full-document replacement: the editor owns the whole file; do not
|
||||
# merge omitted sections back from disk (#62723).
|
||||
approvals_mode_changed = _approval_mode_of(parsed) != _approval_mode_of(read_raw_config())
|
||||
save_config(parsed, merge_existing=False)
|
||||
# Same indicator refresh as the schema-driven save above.
|
||||
if approvals_mode_changed and not _is_other_profile(body.profile or profile):
|
||||
_broadcast_gateway_session_info()
|
||||
return {"ok": True}
|
||||
|
||||
try:
|
||||
|
||||
@@ -0,0 +1,226 @@
|
||||
"""Approvals config saves must re-emit session.info to live gateway sessions.
|
||||
|
||||
Regression for the desktop "YOLO toggle does nothing / flips back off" bug:
|
||||
the settings page saves ``approvals.mode`` through REST ``PUT /api/config``
|
||||
(and the raw editor through ``PUT /api/config/raw``), which wrote config.yaml
|
||||
and emitted nothing. Enforcement follows the file immediately (the approval
|
||||
gate re-reads config per command), but every live session's YOLO/approval
|
||||
indicator repaints only on a ``session.info`` event, so the UI kept showing
|
||||
stale bypass state, in the dangerous direction. The ``config.set`` RPC path
|
||||
already re-emits after a mode flip; these tests pin the REST paths to the
|
||||
same contract.
|
||||
"""
|
||||
|
||||
import types
|
||||
|
||||
import pytest
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def client(_isolate_hermes_home):
|
||||
try:
|
||||
from starlette.testclient import TestClient
|
||||
except ImportError:
|
||||
pytest.skip("fastapi/starlette not installed")
|
||||
from hermes_cli import web_server
|
||||
|
||||
client = TestClient(web_server.app)
|
||||
client.headers[web_server._SESSION_HEADER_NAME] = web_server._SESSION_TOKEN
|
||||
return client
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def broadcast_calls(monkeypatch):
|
||||
"""Stub the in-memory gateway module seam and record broadcasts."""
|
||||
import sys
|
||||
|
||||
calls = []
|
||||
# tests/conftest.py's session-reaper teardown walks tui_gateway.server
|
||||
# attributes; the stub must carry an empty _sessions to survive it.
|
||||
stub = types.SimpleNamespace(
|
||||
broadcast_session_info=lambda: calls.append(True),
|
||||
_sessions={},
|
||||
)
|
||||
monkeypatch.setitem(sys.modules, "tui_gateway.server", stub)
|
||||
return calls
|
||||
|
||||
|
||||
class TestApprovalsSaveBroadcast:
|
||||
def test_get_shaped_record_roundtrip_does_not_broadcast(self, client, broadcast_calls):
|
||||
"""The settings page PUTs the defaulted GET record back verbatim on
|
||||
every autosave. That must not broadcast: disk holds sparse YAML while
|
||||
GET returns defaults, so a block-level compare is always-unequal (the
|
||||
review-caught spam bug). Only an effective mode change may emit."""
|
||||
record = client.get("/api/config").json()
|
||||
assert "approvals" in record
|
||||
|
||||
first = client.put("/api/config", json={"config": record})
|
||||
assert first.status_code == 200
|
||||
second = client.put("/api/config", json={"config": record})
|
||||
assert second.status_code == 200
|
||||
assert not broadcast_calls, (
|
||||
"autosaving the unmodified GET record broadcast session.info; "
|
||||
"every settings autosave would walk all live sessions"
|
||||
)
|
||||
|
||||
flipped = {**record, "approvals": {**record["approvals"], "mode": "off"}}
|
||||
resp = client.put("/api/config", json={"config": flipped})
|
||||
assert resp.status_code == 200
|
||||
assert len(broadcast_calls) == 1, (
|
||||
"an actual approvals.mode change in the GET-shaped record must "
|
||||
"broadcast exactly once"
|
||||
)
|
||||
|
||||
def test_approvals_mode_change_broadcasts(self, client, broadcast_calls):
|
||||
resp = client.put("/api/config", json={"config": {"approvals": {"mode": "off"}}})
|
||||
assert resp.status_code == 200
|
||||
assert broadcast_calls, (
|
||||
"PUT /api/config changed approvals.mode but no session.info "
|
||||
"broadcast reached the gateway, so live sessions keep painting "
|
||||
"stale YOLO/approval state"
|
||||
)
|
||||
|
||||
def test_non_approvals_change_does_not_broadcast(self, client, broadcast_calls):
|
||||
resp = client.put("/api/config", json={"config": {"display": {"skin": "mono"}}})
|
||||
assert resp.status_code == 200
|
||||
assert not broadcast_calls, (
|
||||
"a save that never touched approvals must not spam session.info"
|
||||
)
|
||||
|
||||
def test_approvals_noop_save_does_not_broadcast(self, client, broadcast_calls):
|
||||
first = client.put("/api/config", json={"config": {"approvals": {"mode": "off"}}})
|
||||
assert first.status_code == 200
|
||||
broadcast_calls.clear()
|
||||
|
||||
again = client.put("/api/config", json={"config": {"approvals": {"mode": "off"}}})
|
||||
assert again.status_code == 200
|
||||
assert not broadcast_calls, (
|
||||
"saving an identical approvals block is a no-op and must not "
|
||||
"re-emit session.info"
|
||||
)
|
||||
|
||||
def test_own_profile_named_default_broadcasts(self, client, broadcast_calls):
|
||||
"""Dashboard/desktop often send ?profile=default for this process's
|
||||
own home. That is not an other-profile save and must still emit."""
|
||||
resp = client.put(
|
||||
"/api/config?profile=default",
|
||||
json={"config": {"approvals": {"mode": "off"}}},
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
assert broadcast_calls, (
|
||||
"?profile=default is this process's own HERMES_HOME; skipping "
|
||||
"the broadcast leaves live sessions painting stale YOLO state"
|
||||
)
|
||||
|
||||
def test_other_profile_save_does_not_broadcast(self, client, broadcast_calls, monkeypatch, tmp_path):
|
||||
from hermes_cli import web_server
|
||||
|
||||
profile_dir = tmp_path / "profiles" / "other"
|
||||
profile_dir.mkdir(parents=True)
|
||||
monkeypatch.setattr(web_server, "_resolve_profile_dir", lambda name: profile_dir)
|
||||
|
||||
resp = client.put(
|
||||
"/api/config",
|
||||
json={"config": {"approvals": {"mode": "off"}}, "profile": "other"},
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
assert not broadcast_calls, (
|
||||
"a profile-scoped save targets a different HERMES_HOME than this "
|
||||
"process's gateway sessions; broadcasting our own sessions' "
|
||||
"unchanged state is wrong"
|
||||
)
|
||||
|
||||
def test_gateway_not_imported_is_a_noop(self, client, monkeypatch):
|
||||
import sys
|
||||
|
||||
monkeypatch.delitem(sys.modules, "tui_gateway.server", raising=False)
|
||||
resp = client.put("/api/config", json={"config": {"approvals": {"mode": "smart"}}})
|
||||
assert resp.status_code == 200
|
||||
assert "tui_gateway.server" not in sys.modules, (
|
||||
"the broadcast seam must not IMPORT the gateway; a process "
|
||||
"without one has no sessions to notify"
|
||||
)
|
||||
|
||||
def test_raw_save_deleting_approvals_block_broadcasts(self, client, broadcast_calls):
|
||||
seed = client.put(
|
||||
"/api/config/raw",
|
||||
json={"yaml_text": "approvals:\n mode: 'off'\n"},
|
||||
)
|
||||
assert seed.status_code == 200
|
||||
broadcast_calls.clear()
|
||||
|
||||
# Full-document replacement that drops the approvals block entirely:
|
||||
# effective mode falls back to default (manual), so indicators must
|
||||
# repaint.
|
||||
resp = client.put(
|
||||
"/api/config/raw",
|
||||
json={"yaml_text": "display:\n skin: default\n"},
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
assert broadcast_calls, (
|
||||
"deleting the approvals block changes the effective mode and "
|
||||
"must broadcast"
|
||||
)
|
||||
|
||||
def test_raw_save_approvals_change_broadcasts(self, client, broadcast_calls):
|
||||
resp = client.put(
|
||||
"/api/config/raw",
|
||||
json={"yaml_text": "approvals:\n mode: 'off'\n"},
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
assert broadcast_calls, (
|
||||
"PUT /api/config/raw changed approvals but no session.info "
|
||||
"broadcast reached the gateway"
|
||||
)
|
||||
|
||||
def test_raw_save_without_approvals_change_does_not_broadcast(self, client, broadcast_calls):
|
||||
seed = client.put(
|
||||
"/api/config/raw",
|
||||
json={"yaml_text": "approvals:\n mode: manual\ndisplay:\n skin: default\n"},
|
||||
)
|
||||
assert seed.status_code == 200
|
||||
broadcast_calls.clear()
|
||||
|
||||
resp = client.put(
|
||||
"/api/config/raw",
|
||||
json={"yaml_text": "approvals:\n mode: manual\ndisplay:\n skin: mono\n"},
|
||||
)
|
||||
assert resp.status_code == 200
|
||||
assert not broadcast_calls
|
||||
|
||||
|
||||
class TestGatewayBroadcastHelper:
|
||||
def test_broadcast_session_info_emits_for_live_sessions(self, _isolate_hermes_home, monkeypatch):
|
||||
"""tui_gateway.server.broadcast_session_info walks _sessions and emits."""
|
||||
from tui_gateway import server
|
||||
|
||||
emitted = []
|
||||
monkeypatch.setattr(
|
||||
server, "_emit_session_info_for_session",
|
||||
lambda sid, sess: emitted.append(sid),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
server, "_sessions",
|
||||
{"s1": {"agent": object()}, "s2": {"agent": object()}},
|
||||
)
|
||||
|
||||
server.broadcast_session_info()
|
||||
|
||||
assert sorted(emitted) == ["s1", "s2"]
|
||||
|
||||
def test_approvals_slash_mirror_broadcasts(self, _isolate_hermes_home, monkeypatch):
|
||||
"""/approvals <mode> through the slash worker persists config out of
|
||||
band; the mirror must repaint live sessions and the bare read-only
|
||||
form must not."""
|
||||
from tui_gateway import server
|
||||
|
||||
calls = []
|
||||
monkeypatch.setattr(server, "broadcast_session_info", lambda: calls.append(True))
|
||||
|
||||
session = {"agent": None}
|
||||
server._mirror_slash_side_effects("sid1", session, "/approvals off")
|
||||
assert calls, "/approvals <mode> writes approvals.mode and must broadcast"
|
||||
|
||||
calls.clear()
|
||||
server._mirror_slash_side_effects("sid1", session, "/approvals")
|
||||
assert not calls, "bare /approvals only reads the mode"
|
||||
@@ -5776,6 +5776,20 @@ def _emit_session_info_for_session(sid: str, session: dict) -> None:
|
||||
pass
|
||||
|
||||
|
||||
def broadcast_session_info() -> None:
|
||||
"""Re-emit ``session.info`` to every live session.
|
||||
|
||||
For approvals-config writers that bypass the ``config.set`` RPC (which
|
||||
re-emits itself): the REST config saves and the ``/approvals`` slash
|
||||
mirror. Only reaches sessions in THIS process; a spawned
|
||||
``tui_gateway.entry`` child gateway has its own ``_sessions``.
|
||||
"""
|
||||
with _sessions_lock:
|
||||
sessions = list(_sessions.items())
|
||||
for sid, sess in sessions:
|
||||
_emit_session_info_for_session(sid, sess)
|
||||
|
||||
|
||||
# Tool Args/Result text shipped to the TUI for the verbose trail line. The TUI
|
||||
# renders only a small persisted preview (ui-tui VERBOSE_TRAIL_MAX_CHARS), kept
|
||||
# all session and expanded by default — so shipping more than that is pure pipe
|
||||
@@ -13939,6 +13953,10 @@ def _mirror_slash_side_effects(sid: str, session: dict, command: str) -> str:
|
||||
if name == "model" and arg and agent:
|
||||
result = _apply_model_switch(sid, session, arg)
|
||||
return result.get("warning", "")
|
||||
elif name == "approvals" and arg:
|
||||
# The slash worker already persisted the new approvals.mode; the
|
||||
# bare (read-only) form has no arg and needs no repaint.
|
||||
broadcast_session_info()
|
||||
elif name == "personality" and arg and agent:
|
||||
pname, new_prompt = _validate_personality(arg, _load_cfg())
|
||||
# Persist through the single owner so this surface can never
|
||||
|
||||
Reference in New Issue
Block a user