From 2e320e6d1a81bae96ac9b7ccf07d10c92120ca94 Mon Sep 17 00:00:00 2001 From: brooklyn! Date: Wed, 16 Sep 2026 00:02:11 -0700 Subject: [PATCH 01/18] fix(desktop): keep inline edit placeholders off entered text --- .../thread/user-edit-composer.tsx | 8 +++- .../thread/user-message-edit.test.tsx | 47 +++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/apps/desktop/src/components/assistant-ui/thread/user-edit-composer.tsx b/apps/desktop/src/components/assistant-ui/thread/user-edit-composer.tsx index 9ae6f5fa43..460c464501 100644 --- a/apps/desktop/src/components/assistant-ui/thread/user-edit-composer.tsx +++ b/apps/desktop/src/components/assistant-ui/thread/user-edit-composer.tsx @@ -36,8 +36,10 @@ import { } from '@/app/chat/composer/inline-refs' import { chipTypedPathOnSpace, pathifyRefs } from '@/app/chat/composer/path-refs' import { + beginComposerComposition, composerPlainText, insertComposerContentsAtCaret, + markEditorEmptiness, placeCaretEnd, refChipElement, renderComposerContents, @@ -257,6 +259,9 @@ export const UserEditComposer: FC = ({ cwd, gateway, sess const syncDraftFromEditor = useCallback( (editor: HTMLDivElement) => { + // Native edits bypass renderComposerContents, so refresh the placeholder + // marker here as well, just like the main composer. + markEditorEmptiness(editor) const nextDraft = sanitizeComposerInput(composerPlainText(editor)) if (nextDraft !== draftRef.current) { @@ -854,8 +859,9 @@ export const UserEditComposer: FC = ({ cwd, gateway, sess composingRef.current = false flushEditorToDraft(event.currentTarget) }} - onCompositionStart={() => { + onCompositionStart={event => { composingRef.current = true + beginComposerComposition(event.currentTarget) }} onDragOver={handleDragOver} onDrop={handleDrop} diff --git a/apps/desktop/src/components/assistant-ui/thread/user-message-edit.test.tsx b/apps/desktop/src/components/assistant-ui/thread/user-message-edit.test.tsx index 98449a0cb3..f747100238 100644 --- a/apps/desktop/src/components/assistant-ui/thread/user-message-edit.test.tsx +++ b/apps/desktop/src/components/assistant-ui/thread/user-message-edit.test.tsx @@ -87,6 +87,53 @@ describe('click-to-edit user message', () => { }) }) + it('hides the placeholder when a cleared inline edit receives text again', async () => { + render( {}} />) + fireEvent.click(await screen.findByRole('button', { name: 'Edit message' })) + + const editor = await screen.findByRole('textbox', { name: 'Edit message' }) + // jsdom does not make contenteditable focusable like Chromium does. + editor.tabIndex = 0 + editor.focus() + expect(document.activeElement).toBe(editor) + + editor.replaceChildren() + fireEvent.input(editor) + await waitFor(() => expect(editor.hasAttribute('data-empty')).toBe(true)) + + editor.textContent = 'fade' + fireEvent.input(editor) + await waitFor(() => expect(editor.matches(':is(:empty, [data-empty])')).toBe(false)) + + editor.replaceChildren() + fireEvent.input(editor) + await waitFor(() => expect(editor.hasAttribute('data-empty')).toBe(true)) + fireEvent.paste(editor, { clipboardData: { getData: () => 'pasted edit' } }) + expect(editor.textContent).toBe('pasted edit') + expect(editor.matches(':is(:empty, [data-empty])')).toBe(false) + }) + + it('hides the inline edit placeholder during IME preedit and restores it on cancellation', async () => { + render( {}} />) + fireEvent.click(await screen.findByRole('button', { name: 'Edit message' })) + const editor = await screen.findByRole('textbox', { name: 'Edit message' }) + editor.tabIndex = 0 + editor.focus() + + editor.replaceChildren() + fireEvent.input(editor) + await waitFor(() => expect(editor.hasAttribute('data-empty')).toBe(true)) + + fireEvent.compositionStart(editor) + editor.textContent = 'に' + fireEvent.input(editor) + expect(editor.matches(':is(:empty, [data-empty])')).toBe(false) + + editor.replaceChildren() + fireEvent.compositionEnd(editor) + expect(editor.matches(':is(:empty, [data-empty])')).toBe(true) + }) + it('does not submit an inline edit while IME composition is active', async () => { const onEdit = vi.fn(async (_message: AppendMessage) => {}) From 4ba717df126c5266009ef9cad242411fddfe599a Mon Sep 17 00:00:00 2001 From: xielevi <212198284+xielevi@users.noreply.github.com> Date: Tue, 15 Sep 2026 22:39:04 +0800 Subject: [PATCH 02/18] fix(profiles): migrate session/routing identity on profile rename MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Renaming a profile moved profiles// to profiles//, so the row DATA travelled with the directory, but the profile name is also baked into keys/values the move left untouched: session keys (agent::* namespace), sessions.profile_name (fail-closed owner ladder / Desktop sidebar scope / @session: deep links), sessions.origin_json.profile, gateway_heartbeats.profile, delivery_obligations (session_key + adapter_profile), telegram_dm_topic_* profile_name bindings, and the gateway_routing index. Left stale, every inbound event on a chat keyed to the old name resolved to a profile that no longer exists — flooding errors.log with "Profile does not exist ... falling back to global HERMES_HOME" every few seconds — and renamed sessions dropped out of the sidebar / broke their deep links. The routing index is held in memory by a live multiplexer and written back periodically, so a CLI-side DB rewrite alone is clobbered. Fix in layers: - SessionDB.rekey_profile_state: atomic durable rewrite of the state.db tables, matching the agent:: namespace by exact prefix (substr, not LIKE — '_' is a legal profile-name character and a LIKE wildcard), rewriting the profile inside routing/origin JSON, and REFUSING on a target collision (routing rows or telegram bindings) instead of silently merging. - SessionStore.rekey_profile_routing: rekey the in-memory routing index (keys + origin.profile) then persist — the half a DB write cannot reach. Raises on a target-key collision before mutating. - Control verb migrate-profile-identity (params-carrying; the socket passes params only to handlers that declare them, bare handlers unchanged) so a live gateway rekeys its in-memory copy AND both durable stores (routing home + the renamed profile's own state.db). - rename_profile calls the verb when a multiplexer is live and, if it fails, does NOT fall back to a racing CLI-side write: it prints a warning telling the operator to restart the gateway and retry. With no live gateway it performs the durable rewrite itself (safe: nothing else holds the store open). Checkpoints keyed by the profile's workdir path are a known related gap, tracked separately, not addressed here. Tests: rekey_profile_state (all tables, routing/origin JSON, collisions, idempotent, no-op), rekey_profile_routing (namespace + origin, no-op, no overwrite), control verb param passing, and rename end-to-end for both the live-gateway (delegates, refuses unsafe fallback) and no-gateway (durable rewrite) paths. --- gateway/control_socket.py | 32 +++- gateway/run.py | 36 +++- gateway/session.py | 27 +++ hermes_cli/profiles.py | 48 +++++- hermes_state_gateway.py | 107 ++++++++++++ tests/gateway/test_control_socket.py | 33 ++++ tests/gateway/test_rekey_profile_routing.py | 77 +++++++++ tests/hermes_cli/test_profiles.py | 71 ++++++++ .../hermes_state/test_rekey_profile_state.py | 157 ++++++++++++++++++ 9 files changed, 580 insertions(+), 8 deletions(-) create mode 100644 tests/gateway/test_rekey_profile_routing.py create mode 100644 tests/hermes_state/test_rekey_profile_state.py diff --git a/gateway/control_socket.py b/gateway/control_socket.py index b45608a9dd..1845936afc 100644 --- a/gateway/control_socket.py +++ b/gateway/control_socket.py @@ -12,6 +12,7 @@ from __future__ import annotations import asyncio import contextlib import hashlib +import inspect import json import logging import os @@ -124,7 +125,7 @@ class GatewayControlServer: because its control socket couldn't bind; consumers fall back to the scan layer.""" def __init__(self, home: Optional[Path] = None, *, - verb_handlers: Optional[dict[str, Callable[[], dict[str, Any]]]] = None) -> None: + verb_handlers: Optional[dict[str, Callable[..., dict[str, Any]]]] = None) -> None: if home is None: from gateway.status import _get_process_hermes_home home = _get_process_hermes_home() @@ -133,7 +134,7 @@ class GatewayControlServer: self._pipe_server: Any = None # Windows proactor pipe server self._bind_path: Optional[Path] = None self._pointer_file: Optional[Path] = None - self._handlers: dict[str, Callable[[], dict[str, Any]]] = { + self._handlers: dict[str, Callable[..., dict[str, Any]]] = { "identify": build_identify_payload, "status": build_status_payload, **(verb_handlers or {})} async def start(self) -> bool: @@ -210,7 +211,12 @@ class GatewayControlServer: response: dict[str, Any] = {"ok": False, "error": f"unknown verb: {verb!r}", "protocol": CONTROL_PROTOCOL_VERSION, "supported_verbs": sorted(self._handlers)} else: - response = {"ok": True, "protocol": CONTROL_PROTOCOL_VERSION, "result": handler()} + # Verbs that carry arguments (e.g. migrate-profile-identity) declare a ``params`` + # parameter; argument-less verbs (identify/status/rescan) keep their bare signature. + params = request.get("params") if isinstance(request.get("params"), dict) else {} + wants_params = "params" in inspect.signature(handler).parameters + response = {"ok": True, "protocol": CONTROL_PROTOCOL_VERSION, + "result": handler(params) if wants_params else handler()} except Exception as exc: response = {"ok": False, "error": f"{type(exc).__name__}: {exc}", "protocol": CONTROL_PROTOCOL_VERSION} if request_id is not None: @@ -264,11 +270,15 @@ class _PipeControlProtocol(asyncio.Protocol): self._transport.close() -def query_gateway_control(home: Path, verb: str, *, timeout: float = _DEFAULT_CLIENT_TIMEOUT) -> Optional[dict[str, Any]]: +def query_gateway_control(home: Path, verb: str, *, params: Optional[dict[str, Any]] = None, + timeout: float = _DEFAULT_CLIENT_TIMEOUT) -> Optional[dict[str, Any]]: """Ask the gateway serving ``home`` a control verb; returns its ``result`` payload. Any failure (no/stale socket, timeout, malformed answer, ``ok: false``) returns None so callers fall back to the scan layer. - Never raises.""" - request = json.dumps({"verb": verb, "id": 1, "protocol": CONTROL_PROTOCOL_VERSION}).encode("utf-8") + b"\n" + ``params`` carries verb arguments (e.g. ``{"old": ..., "new": ...}``). Never raises.""" + payload: dict[str, Any] = {"verb": verb, "id": 1, "protocol": CONTROL_PROTOCOL_VERSION} + if params: + payload["params"] = params + request = json.dumps(payload).encode("utf-8") + b"\n" query = _query_windows_pipe if _IS_WINDOWS else _query_unix_socket try: raw = query(Path(home), request, timeout) @@ -348,3 +358,13 @@ def rescan_gateway_profiles(home: Path, *, timeout: float = 8.0) -> Optional[dic when no gateway answers / the gateway predates the verb — callers then rely on the periodic rescan (or the restart reminder).""" return query_gateway_control(home, "rescan-profiles", timeout=timeout) + + +def migrate_gateway_profile_identity(home: Path, old_name: str, new_name: str, *, + timeout: float = 8.0) -> Optional[dict[str, Any]]: + """Ask the multiplexer serving ``home`` to rekey a renamed profile's in-memory + on-disk routing + from ``agent::`` to ``agent::`` now. Returns its ``{"rekeyed": N, ...}`` answer, or None + when no gateway answers / the gateway predates the verb — the CLI's durable DB rewrite still lands, + and a restart reconciles the in-memory copy.""" + return query_gateway_control(home, "migrate-profile-identity", + params={"old": old_name, "new": new_name}, timeout=timeout) diff --git a/gateway/run.py b/gateway/run.py index 339079fb83..26a3a67f59 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -5116,9 +5116,43 @@ async def _start_gateway_start_control_socket(runner): except concurrent.futures.TimeoutError: return {"multiplex": True, "pending": True, "served_profiles": runner.served_profile_names()} + def _migrate_profile_identity_handler(params: dict) -> dict: + """Migrate both durable stores and the routing index owned by this live gateway.""" + old, new = str(params.get("old") or "").strip(), str(params.get("new") or "").strip() + if not old or not new or old == new: + return {"ok": False, "error": "old/new required and must differ"} + store = getattr(runner, "session_store", None) + if store is None: + return {"ok": False, "error": "live gateway has no session store"} + acquired = [] + try: + from hermes_state_registry import acquire, release_or_close + db_counts: dict[str, dict[str, int]] = {} + routing_db = getattr(store, "_routing_db", None) + if routing_db is not None and hasattr(routing_db, "rekey_profile_state"): + db_counts["routing"] = routing_db.rekey_profile_state(old, new) + routing_home = getattr(store, "_routing_home", None) + profile_path = Path(routing_home) / "profiles" / new / "state.db" if routing_home else None + if profile_path is not None and profile_path.exists(): + profile_db = acquire(profile_path) + acquired.append(profile_db) + db_counts["profile"] = profile_db.rekey_profile_state(old, new) + rekeyed = store.rekey_profile_routing(old, new) + return {"ok": True, "rekeyed": rekeyed, "db": db_counts} + except Exception as exc: + logger.warning("Profile identity migration failed for %r->%r: %s", old, new, exc) + return {"ok": False, "error": f"{type(exc).__name__}: {exc}"} + finally: + for db in acquired: + try: + release_or_close(db) + except Exception: + logger.debug("Failed to release renamed profile state DB", exc_info=True) + _control_server = GatewayControlServer( verb_handlers={"pause-for-update": _pause_for_update_handler, - "rescan-profiles": _rescan_profiles_handler}) + "rescan-profiles": _rescan_profiles_handler, + "migrate-profile-identity": _migrate_profile_identity_handler}) if not await _control_server.start(): _control_server = None else: diff --git a/gateway/session.py b/gateway/session.py index fb074d0cc6..59cda21e99 100644 --- a/gateway/session.py +++ b/gateway/session.py @@ -1121,6 +1121,33 @@ class SessionStore( self._save() return new_entry + def rekey_profile_routing(self, old_name: str, new_name: str) -> int: + """Rekey the live routing index and reject target collisions before mutation.""" + from dataclasses import replace as _dc_replace + old, new = (old_name or "").strip(), (new_name or "").strip() + if not old or not new or old == new: + return 0 + old_ns, new_ns = f"agent:{old}:", f"agent:{new}:" + with self._lock: + moving = [key for key in self._entries if key.startswith(old_ns)] + collisions = [ + new_ns + key[len(old_ns):] for key in moving + if new_ns + key[len(old_ns):] in self._entries] + if collisions: + raise ValueError( + f"profile routing collision while renaming {old!r} to {new!r}: " + f"{collisions[0]!r} already exists") + for key in moving: + new_key = new_ns + key[len(old_ns):] + entry = self._entries.pop(key) + origin = entry.origin + if origin is not None and getattr(origin, "profile", None) == old: + origin = _dc_replace(origin, profile=new) + self._entries[new_key] = _dc_replace(entry, session_key=new_key, origin=origin) + if moving: + self._save() + return len(moving) + # Compression repoint is store bookkeeping, not user activity — leave ``updated_at`` alone so a # background compression on an idle session cannot make it look fresh to the # restart-resume freshness gate (#85709). diff --git a/hermes_cli/profiles.py b/hermes_cli/profiles.py index aa7cf4feaf..3cf198d06e 100644 --- a/hermes_cli/profiles.py +++ b/hermes_cli/profiles.py @@ -1864,12 +1864,58 @@ def rename_profile(old_name: str, new_name: str) -> Path: # 5. Update active_profile if it pointed to old name _retarget_active_profile(old_canon, new_canon, f"✓ Active profile updated: {new_canon}") - # 6. Hot-serve the renamed profile now (mirrors create; a missed signal only delays it). + # 6. Migrate profile-name-keyed session/routing state (session keys, profile_name, heartbeats, + # delivery + routing index) from the old name to the new one. A stale ``agent::*`` routing + # key otherwise resolves to a profile that no longer exists on every inbound event. + _migrate_profile_identity(old_canon, new_canon, live_mux) + + # 7. Hot-serve the renamed profile now (mirrors create; a missed signal only delays it). if live_mux: _notify_multiplexer(new_canon) return new_dir +def _migrate_profile_identity(old_canon: str, new_canon: str, live_mux: bool) -> None: + """Rekey renamed-profile identity without racing a live gateway's in-memory routing index.""" + if live_mux: + try: + from hermes_constants import get_default_hermes_root + from gateway.control_socket import migrate_gateway_profile_identity + answer = migrate_gateway_profile_identity( + get_default_hermes_root(), old_canon, new_canon) + except Exception as exc: + answer, failure = None, f"{type(exc).__name__}: {exc}" + else: + failure = answer.get("error") if isinstance(answer, dict) else None + if isinstance(answer, dict) and answer.get("ok") is True: + return + detail = f" ({failure})" if failure else "" + print( + "⚠ Profile was renamed, but the live gateway could not migrate session identity" + f"{detail}. Restart the gateway, then retry the identity migration.", + file=sys.stderr) + return + + from hermes_state_registry import acquire, release_or_close + from hermes_constants import get_default_hermes_root + root = get_default_hermes_root() + for db_path in (root / "state.db", get_profile_dir(new_canon) / "state.db"): + if not db_path.exists(): + continue + db = None + try: + db = acquire(db_path) + db.rekey_profile_state(old_canon, new_canon) + except Exception as exc: + click.echo( + f"⚠ Profile was renamed, but identity migration failed for {db_path}: " + f"{type(exc).__name__}: {exc}", err=True) + finally: + if db is not None: + with contextlib.suppress(Exception): + release_or_close(db) + + # Profile env resolution (called from _apply_profile_override) def resolve_profile_env(profile_name: str) -> str: diff --git a/hermes_state_gateway.py b/hermes_state_gateway.py index 29905eddd6..e12600b52f 100644 --- a/hermes_state_gateway.py +++ b/hermes_state_gateway.py @@ -546,6 +546,113 @@ class SessionGatewayMixin: return self._write_sql("DELETE FROM gateway_hygiene_state WHERE session_key = ?", (session_key,)) + def rekey_profile_state(self, old_name: str, new_name: str) -> Dict[str, int]: + """Atomically rewrite exact profile identity in this state database.""" + old, new = (old_name or "").strip(), (new_name or "").strip() + counts: Dict[str, int] = {} + if not old or not new or old == new: + return counts + old_ns, new_ns = f"agent:{old}:", f"agent:{new}:" + ns_len = len(old_ns) + + def _do(conn): + existing = {row[0] for row in conn.execute( + "SELECT name FROM sqlite_master WHERE type='table'").fetchall()} + collision = conn.execute( + "SELECT old.scope, ? || substr(old.session_key, ?) " + "FROM gateway_routing AS old JOIN gateway_routing AS target " + "ON target.scope = old.scope " + "AND target.session_key = ? || substr(old.session_key, ?) " + "WHERE substr(old.session_key, 1, ?) = ? LIMIT 1", + (new_ns, ns_len + 1, new_ns, ns_len + 1, ns_len, old_ns), + ).fetchone() + if collision is not None: + raise ValueError( + f"profile routing collision in scope {collision[0]!r}: {collision[1]!r}") + for table, columns in ( + ("telegram_dm_topic_mode", ("chat_id",)), + ("telegram_dm_topic_bindings", ("chat_id", "thread_id")), + ): + if table not in existing: + continue + equality = " AND ".join( + f"target.{column} = old.{column}" for column in columns) + collision = conn.execute( + f"SELECT 1 FROM {table} AS old JOIN {table} AS target " + f"ON target.profile_name = ? AND {equality} " + "WHERE old.profile_name = ? LIMIT 1", (new, old)).fetchone() + if collision is not None: + raise ValueError(f"profile identity collision in {table}") + + counts["sessions_profile_name"] = conn.execute( + "UPDATE sessions SET profile_name = ? WHERE profile_name = ?", (new, old)).rowcount + counts["gateway_heartbeats_profile"] = conn.execute( + "UPDATE gateway_heartbeats SET profile = ? WHERE profile = ?", (new, old)).rowcount + counts["sessions_session_key"] = conn.execute( + "UPDATE sessions SET session_key = ? || substr(session_key, ?) " + "WHERE substr(session_key, 1, ?) = ?", + (new_ns, ns_len + 1, ns_len, old_ns)).rowcount + + origin_count = 0 + for session_id, origin_json in conn.execute( + "SELECT id, origin_json FROM sessions WHERE origin_json IS NOT NULL").fetchall(): + try: + payload = json.loads(origin_json) + except (ValueError, TypeError): + continue + if isinstance(payload, dict) and payload.get("profile") == old: + payload["profile"] = new + conn.execute("UPDATE sessions SET origin_json = ? WHERE id = ?", + (json.dumps(payload, ensure_ascii=False), session_id)) + origin_count += 1 + counts["sessions_origin_json"] = origin_count + + if "delivery_obligations" in existing: + counts["delivery_obligations_adapter_profile"] = conn.execute( + "UPDATE delivery_obligations SET adapter_profile = ? WHERE adapter_profile = ?", + (new, old)).rowcount + counts["delivery_obligations_session_key"] = conn.execute( + "UPDATE delivery_obligations SET session_key = ? || substr(session_key, ?) " + "WHERE substr(session_key, 1, ?) = ?", + (new_ns, ns_len + 1, ns_len, old_ns)).rowcount + for table in ("telegram_dm_topic_mode", "telegram_dm_topic_bindings"): + if table in existing: + counts[f"{table}_profile_name"] = conn.execute( + f"UPDATE {table} SET profile_name = ? WHERE profile_name = ?", + (new, old)).rowcount + if "telegram_dm_topic_bindings" in existing: + counts["telegram_dm_topic_bindings_session_key"] = conn.execute( + "UPDATE telegram_dm_topic_bindings " + "SET session_key = ? || substr(session_key, ?) " + "WHERE substr(session_key, 1, ?) = ?", + (new_ns, ns_len + 1, ns_len, old_ns)).rowcount + + routing = conn.execute( + "SELECT rowid, session_key, entry_json FROM gateway_routing " + "WHERE substr(session_key, 1, ?) = ?", (ns_len, old_ns)).fetchall() + for rowid, session_key, entry_json in routing: + new_session_key = new_ns + session_key[ns_len:] + new_json = entry_json + if entry_json: + try: + payload = json.loads(entry_json) + except (ValueError, TypeError): + payload = None + if isinstance(payload, dict): + if isinstance(payload.get("session_key"), str) and payload["session_key"].startswith(old_ns): + payload["session_key"] = new_ns + payload["session_key"][ns_len:] + origin = payload.get("origin") + if isinstance(origin, dict) and origin.get("profile") == old: + origin["profile"] = new + new_json = json.dumps(payload, ensure_ascii=False) + conn.execute( + "UPDATE gateway_routing SET session_key = ?, entry_json = ? WHERE rowid = ?", + (new_session_key, new_json, rowid)) + counts["gateway_routing"] = len(routing) + + self._execute_write(_do) + return counts + @staticmethod def session_gateway_runtime(session_meta: Optional[Dict[str, Any]]) -> Dict[str, Any]: """Read the persisted runtime route off a session row dict (``model_config`` as diff --git a/tests/gateway/test_control_socket.py b/tests/gateway/test_control_socket.py index b4034ebb0c..825f73b4f3 100644 --- a/tests/gateway/test_control_socket.py +++ b/tests/gateway/test_control_socket.py @@ -161,6 +161,39 @@ def test_unknown_verb_and_malformed_request(home: Path): assert payload["protocol"] == CONTROL_PROTOCOL_VERSION +def test_verb_handler_receives_params(home: Path): + """A handler declaring a ``params`` argument is called with the request's params dict; a bare + handler is still called with no args (backward compat for identify/status/rescan).""" + received = {} + + def with_params(params): + received.update(params) + return {"echo": params} + + def bare(): + return {"ok": 1} + + async def scenario(): + server = GatewayControlServer( + home, verb_handlers={"with-params": with_params, "bare": bare}) + assert await server.start() + try: + loop = asyncio.get_running_loop() + got = await loop.run_in_executor( + None, lambda: query_gateway_control( + home, "with-params", params={"old": "a", "new": "b"})) + bare_ok = await loop.run_in_executor( + None, lambda: query_gateway_control(home, "bare")) + return got, bare_ok + finally: + await server.stop() + + got, bare_ok = _run(scenario()) + assert got == {"echo": {"old": "a", "new": "b"}} + assert received == {"old": "a", "new": "b"} + assert bare_ok == {"ok": 1} + + def test_stop_removes_socket_and_pointer(home: Path): async def scenario(): server = GatewayControlServer( diff --git a/tests/gateway/test_rekey_profile_routing.py b/tests/gateway/test_rekey_profile_routing.py new file mode 100644 index 0000000000..daef3d6d97 --- /dev/null +++ b/tests/gateway/test_rekey_profile_routing.py @@ -0,0 +1,77 @@ +"""In-memory routing rekey for `hermes profile rename`. + +The routing index lives in ``SessionStore._entries`` and is written back periodically, so a durable +DB rewrite alone is clobbered — the live store must rekey its in-memory copy too. This is why a +renamed profile's old namespace kept resurfacing until the gateway restarted. +""" +from __future__ import annotations + + +def _make_store(tmp_path): + from gateway.config import GatewayConfig + from gateway.session import SessionStore + sessions_dir = tmp_path / "sessions" + sessions_dir.mkdir() + store = SessionStore( + sessions_dir, + GatewayConfig(sessions_dir=sessions_dir, write_sessions_json=False, + multiplex_profiles=True), + ) + store._ensure_loaded() + return store + + +def _entry(session_key, chat_id, profile): + from gateway.session import SessionEntry, SessionSource, Platform + from gateway.session_lifecycle import _now + now = _now() + return SessionEntry( + session_key=session_key, session_id=f"sid-{chat_id}", + platform=Platform.FEISHU, chat_type="dm", created_at=now, updated_at=now, + origin=SessionSource(platform=Platform.FEISHU, chat_id=chat_id, profile=profile), + ) + + +def test_rekeys_old_namespace_and_origin_profile(tmp_path): + store = _make_store(tmp_path) + with store._lock: + store._entries["agent:oldname:feishu:dm:chatA"] = _entry( + "agent:oldname:feishu:dm:chatA", "chatA", "oldname") + store._entries["agent:keepme:feishu:dm:chatB"] = _entry( + "agent:keepme:feishu:dm:chatB", "chatB", "keepme") + + moved = store.rekey_profile_routing("oldname", "newname") + assert moved == 1 + + assert "agent:oldname:feishu:dm:chatA" not in store._entries + new_entry = store._entries["agent:newname:feishu:dm:chatA"] + assert new_entry.session_key == "agent:newname:feishu:dm:chatA" + assert new_entry.origin.profile == "newname" + # Bystander namespace untouched. + assert store._entries["agent:keepme:feishu:dm:chatB"].origin.profile == "keepme" + + +def test_noop_for_equal_or_empty_names(tmp_path): + store = _make_store(tmp_path) + with store._lock: + store._entries["agent:oldname:feishu:dm:chatA"] = _entry( + "agent:oldname:feishu:dm:chatA", "chatA", "oldname") + assert store.rekey_profile_routing("x", "x") == 0 + assert store.rekey_profile_routing("", "y") == 0 + assert "agent:oldname:feishu:dm:chatA" in store._entries + + +def test_does_not_overwrite_existing_new_namespace_key(tmp_path): + store = _make_store(tmp_path) + with store._lock: + store._entries["agent:oldname:feishu:dm:chatA"] = _entry( + "agent:oldname:feishu:dm:chatA", "chatA", "oldname") + # A collision on the target key (should not happen in practice) is left alone. + store._entries["agent:newname:feishu:dm:chatA"] = _entry( + "agent:newname:feishu:dm:chatA", "chatA", "newname") + + import pytest + with pytest.raises(ValueError, match="routing collision"): + store.rekey_profile_routing("oldname", "newname") + assert "agent:oldname:feishu:dm:chatA" in store._entries + assert "agent:newname:feishu:dm:chatA" in store._entries diff --git a/tests/hermes_cli/test_profiles.py b/tests/hermes_cli/test_profiles.py index 010d32d776..b8d8203228 100644 --- a/tests/hermes_cli/test_profiles.py +++ b/tests/hermes_cli/test_profiles.py @@ -867,6 +867,77 @@ class TestRenameProfile: assert not (tmp_path / ".hermes" / "profiles" / ".deleted").exists() assert not old_dir.exists() and new_dir.is_dir() + def test_rename_migrates_session_identity_without_live_gateway(self, profile_env): + """No live gateway → the CLI performs the durable rekey itself so a renamed profile's session + keys / profile_name / routing rows follow the new name (else inbound events on the old name's + chats resolve to a nonexistent profile and flood errors.log).""" + from hermes_state import SessionDB + tmp_path = profile_env + create_profile("oldname", no_alias=True) + old_dir = tmp_path / ".hermes" / "profiles" / "oldname" + # Seed a session owned by the old profile in the profile's own store + the root routing index. + pdb = SessionDB(old_dir / "state.db") + pdb.create_session( + "sess1", "feishu", session_key="agent:oldname:feishu:dm:chatA", + profile_name="oldname", chat_id="chatA", chat_type="dm") + pdb.close() + root_db = SessionDB(tmp_path / ".hermes" / "state.db") + root_db.save_gateway_routing_entry( + "agent:oldname:feishu:dm:chatA", + json.dumps({"session_key": "agent:oldname:feishu:dm:chatA", "session_id": "sess1", + "origin": {"platform": "feishu", "chat_id": "chatA", "profile": "oldname"}}), + scope=str(tmp_path / ".hermes" / "sessions")) + root_db.close() + + with patch("hermes_cli.profiles.check_alias_collision", return_value="skip"), \ + patch("hermes_cli.profiles._live_default_multiplexer", return_value=False): + rename_profile("oldname", "newname") + + new_dir = tmp_path / ".hermes" / "profiles" / "newname" + moved_db = SessionDB(new_dir / "state.db") + row = moved_db._read_one( + "SELECT session_key, profile_name FROM sessions WHERE id = ?", ("sess1",)) + assert row["session_key"] == "agent:newname:feishu:dm:chatA" + assert row["profile_name"] == "newname" + moved_db.close() + root_db2 = SessionDB(tmp_path / ".hermes" / "state.db") + routing = root_db2.load_gateway_routing_entries( + scope=str(tmp_path / ".hermes" / "sessions")) + assert "agent:oldname:feishu:dm:chatA" not in routing + assert "agent:newname:feishu:dm:chatA" in routing + root_db2.close() + + def test_rename_delegates_identity_migration_to_live_gateway(self, profile_env): + """Under a live multiplexer the CLI must NOT rewrite the routing DB directly (the gateway holds + it in memory and would clobber the write); it delegates to the control verb instead.""" + tmp_path = profile_env + create_profile("oldname", no_alias=True) + + with patch("hermes_cli.profiles.check_alias_collision", return_value="skip"), \ + patch("hermes_cli.profiles._live_default_multiplexer", return_value=True), \ + patch("hermes_cli.profiles._notify_multiplexer"), \ + patch("gateway.control_socket.migrate_gateway_profile_identity", + return_value={"ok": True, "rekeyed": 1, "db": {}}) as verb, \ + patch("hermes_state_registry.acquire") as acquire: + rename_profile("oldname", "newname") + + # Delegated to the gateway; the CLI's own durable-rewrite branch never ran. + assert verb.call_count == 1 + assert verb.call_args.args[1:] == ("oldname", "newname") + acquire.assert_not_called() + + + def test_live_gateway_failure_does_not_rewrite_db_directly(self, profile_env, capsys): + create_profile("oldname", no_alias=True) + with patch("hermes_cli.profiles.check_alias_collision", return_value="skip"), \ + patch("hermes_cli.profiles._live_default_multiplexer", return_value=True), \ + patch("hermes_cli.profiles._notify_multiplexer"), \ + patch("gateway.control_socket.migrate_gateway_profile_identity", return_value=None), \ + patch("hermes_state_registry.acquire") as acquire: + rename_profile("oldname", "newname") + acquire.assert_not_called() + assert "Restart the gateway" in capsys.readouterr().err + # =================================================================== # TestExportImport diff --git a/tests/hermes_state/test_rekey_profile_state.py b/tests/hermes_state/test_rekey_profile_state.py new file mode 100644 index 0000000000..307338fbd2 --- /dev/null +++ b/tests/hermes_state/test_rekey_profile_state.py @@ -0,0 +1,157 @@ +"""Regression tests for profile-name-keyed state migration on `hermes profile rename`. + +When a profile is renamed, the directory move carries the row data, but the profile name is also +baked into session keys (``agent::*``), ``sessions.profile_name``, ``gateway_heartbeats.profile``, +``delivery_obligations`` and the ``gateway_routing`` index. Left stale, an inbound event on a chat keyed +to the old name resolves to a profile that no longer exists and floods errors.log. See the profile-rename +identity-migration fix. +""" +import json +import time + +import pytest + +from hermes_state import SessionDB + + +@pytest.fixture +def db(tmp_path): + database = SessionDB(tmp_path / "state.db") + yield database + database.close() + + +class TestRekeyProfileState: + def test_rekeys_session_key_namespace_and_profile_columns(self, db): + # A session owned by the old profile, keyed in its namespace. + db.create_session( + "sess_old", "feishu", session_key="agent:oldname:feishu:dm:chatA", + profile_name="oldname", chat_id="chatA", chat_type="dm", + ) + # An unrelated profile's session must be left untouched. + db.create_session( + "sess_other", "feishu", session_key="agent:keepme:feishu:dm:chatB", + profile_name="keepme", chat_id="chatB", chat_type="dm", + ) + + counts = db.rekey_profile_state("oldname", "newname") + + assert counts["sessions_session_key"] == 1 + assert counts["sessions_profile_name"] == 1 + # Renamed row now lives under the new namespace + owner. + row = db._read_one( + "SELECT session_key, profile_name FROM sessions WHERE id = ?", ("sess_old",)) + assert row["session_key"] == "agent:newname:feishu:dm:chatA" + assert row["profile_name"] == "newname" + # Bystander untouched. + other = db._read_one( + "SELECT session_key, profile_name FROM sessions WHERE id = ?", ("sess_other",)) + assert other["session_key"] == "agent:keepme:feishu:dm:chatB" + assert other["profile_name"] == "keepme" + + def test_rekeys_routing_index_key_and_embedded_profile(self, db): + entry = { + "session_key": "agent:oldname:feishu:dm:chatA", + "session_id": "sess_old", + "origin": {"platform": "feishu", "chat_id": "chatA", "profile": "oldname"}, + } + db.save_gateway_routing_entry( + "agent:oldname:feishu:dm:chatA", json.dumps(entry), scope="/root/sessions") + + counts = db.rekey_profile_state("oldname", "newname") + assert counts["gateway_routing"] == 1 + + rows = db.load_gateway_routing_entries(scope="/root/sessions") + assert "agent:oldname:feishu:dm:chatA" not in rows + assert "agent:newname:feishu:dm:chatA" in rows + payload = json.loads(rows["agent:newname:feishu:dm:chatA"]) + assert payload["session_key"] == "agent:newname:feishu:dm:chatA" + assert payload["origin"]["profile"] == "newname" + + def test_rekeys_heartbeats_and_delivery_obligations(self, db, tmp_path, monkeypatch): + db.register_backend_heartbeat( + backend_id="be1", pid=123, started_at=time.time(), + profile="oldname", host="h") + # delivery_obligations is created lazily by the delivery ledger against the same state.db. + monkeypatch.setenv("HERMES_HOME", str(db.db_path.parent)) + from gateway import delivery_ledger + monkeypatch.setattr(delivery_ledger, "_db_path", lambda: db.db_path) + with delivery_ledger._connect() as conn: + now = time.time() + conn.execute( + "INSERT INTO delivery_obligations (obligation_id, session_key, platform, chat_id, " + "content, state, created_at, updated_at, adapter_profile) " + "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)", + ("ob1", "agent:oldname:feishu:dm:chatA", "feishu", "chatA", + "hi", "pending", now, now, "oldname"), + ) + conn.commit() + + counts = db.rekey_profile_state("oldname", "newname") + + assert counts["gateway_heartbeats_profile"] == 1 + assert counts["delivery_obligations_adapter_profile"] == 1 + assert counts["delivery_obligations_session_key"] == 1 + hb = db._read_one("SELECT profile FROM gateway_heartbeats WHERE backend_id = ?", ("be1",)) + assert hb["profile"] == "newname" + ob = db._read_one( + "SELECT session_key, adapter_profile FROM delivery_obligations WHERE obligation_id = ?", + ("ob1",)) + assert ob["session_key"] == "agent:newname:feishu:dm:chatA" + assert ob["adapter_profile"] == "newname" + + def test_noop_when_names_equal_or_empty(self, db): + assert db.rekey_profile_state("x", "x") == {} + assert db.rekey_profile_state("", "y") == {} + assert db.rekey_profile_state("x", "") == {} + + def test_idempotent(self, db): + db.create_session( + "sess_old", "feishu", session_key="agent:oldname:feishu:dm:chatA", + profile_name="oldname", chat_id="chatA", chat_type="dm", + ) + first = db.rekey_profile_state("oldname", "newname") + assert first["sessions_session_key"] == 1 + second = db.rekey_profile_state("oldname", "newname") + # Nothing left under the old name. + assert second["sessions_session_key"] == 0 + assert second["sessions_profile_name"] == 0 + + def test_underscore_in_profile_name_is_not_a_like_wildcard(self, db): + db.create_session( + "literal", "telegram", session_key="agent:foo_bar:telegram:dm:a", + profile_name="foo_bar", chat_id="a", chat_type="dm") + db.create_session( + "bystander", "telegram", session_key="agent:fooXbar:telegram:dm:b", + profile_name="fooXbar", chat_id="b", chat_type="dm") + db.rekey_profile_state("foo_bar", "renamed") + assert db._read_one( + "SELECT session_key FROM sessions WHERE id = ?", ("literal",) + )["session_key"] == "agent:renamed:telegram:dm:a" + assert db._read_one( + "SELECT session_key FROM sessions WHERE id = ?", ("bystander",) + )["session_key"] == "agent:fooXbar:telegram:dm:b" + + def test_rekeys_origin_json_and_telegram_topic_state(self, db): + db.create_session( + "sess_old", "telegram", session_key="agent:oldname:telegram:dm:chatA", + profile_name="oldname", chat_id="chatA", chat_type="dm") + db._write_sql( + "UPDATE sessions SET origin_json = ? WHERE id = ?", + (json.dumps({"platform": "telegram", "profile": "oldname"}), "sess_old")) + db.enable_telegram_topic_mode( + chat_id="chatA", user_id="userA", profile_name="oldname") + db.bind_telegram_topic( + chat_id="chatA", thread_id="threadA", user_id="userA", + session_key="agent:oldname:telegram:dm:chatA", session_id="sess_old", + profile_name="oldname") + counts = db.rekey_profile_state("oldname", "newname") + row = db._read_one("SELECT origin_json FROM sessions WHERE id = ?", ("sess_old",)) + assert json.loads(row["origin_json"])["profile"] == "newname" + assert counts["sessions_origin_json"] == 1 + binding = db._read_one( + "SELECT profile_name, session_key FROM telegram_dm_topic_bindings " + "WHERE chat_id = ? AND thread_id = ?", ("chatA", "threadA")) + assert binding["profile_name"] == "newname" + assert binding["session_key"] == "agent:newname:telegram:dm:chatA" + From 81140e454623ea8a4ddbbbbc110525de5ea5c34d Mon Sep 17 00:00:00 2001 From: xielevi <212198284+xielevi@users.noreply.github.com> Date: Tue, 15 Sep 2026 23:25:56 +0800 Subject: [PATCH 03/18] fix(profiles): ship a retry path for the rename identity migration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A rename under a live multiplexer that could not reach the control verb warned and stopped there, leaving the operator with no way to finish: the rename cannot be repeated (profiles/ is gone) and the CLI deliberately never rewrites the routing DB a live gateway holds in memory. - `hermes profile migrate-identity `: retries the migration — delegates to the gateway control verb while a multiplexer is live, performs the durable rewrite of both state DBs when none is. Idempotent, and exits non-zero naming the offending database on a collision, a lock, or a partial failure. Only the name format and the existence of the new profile are checked; the old profile directory is expected to be gone. - An older gateway that does not implement the verb is reported as such (`identify` answers while the migrate verb does not), not as "no gateway". - `_migrate_profile_identity` returns an explicit success/failure result so the command can set its exit code; the rename warning now names the exact invocation. - A failed control answer keeps the raw payload when it carries no reason field. - The offline failure branch called `click.echo` in a module that never imports `click`: a failed second database raised NameError instead of printing its warning. --- hermes_cli/profile_cmd.py | 20 ++++- hermes_cli/profiles.py | 83 ++++++++++++++++--- hermes_cli/subcommands/profile.py | 12 +++ .../hermes-agent/references/cli-reference.md | 1 + tests/hermes_cli/test_profiles.py | 78 +++++++++++++++++ website/docs/reference/profile-commands.md | 34 ++++++++ website/docs/user-guide/profiles.md | 1 + 7 files changed, 214 insertions(+), 15 deletions(-) diff --git a/hermes_cli/profile_cmd.py b/hermes_cli/profile_cmd.py index c4d0d8d071..2d355fe721 100644 --- a/hermes_cli/profile_cmd.py +++ b/hermes_cli/profile_cmd.py @@ -9,10 +9,10 @@ from __future__ import annotations from pathlib import Path import os import sys -from typing import Optional +from typing import NoReturn, Optional -def _die(msg: str, code: int = 1, *, err: bool = False) -> None: +def _die(msg: str, code: int = 1, *, err: bool = False) -> NoReturn: print(msg, file=sys.stderr if err else sys.stdout) sys.exit(code) @@ -436,6 +436,21 @@ def _profile_rename(args): _die(f"Error: {e}") +def _profile_migrate_identity(args): + """Retry the identity migration of a rename that already completed. Exits non-zero when a + live gateway would not migrate (it still owns the routing index in memory), or when a + database rejected the rewrite (collision, lock, partial failure).""" + from hermes_cli.profiles import migrate_profile_identity + try: + migrated = migrate_profile_identity(args.old_name, args.new_name) + except (ValueError, FileNotFoundError) as e: + _die(f"Error: {e}") + if not migrated: + _die(f"Error: session identity was not migrated. Restart or stop the gateway, then run:\n" + f" hermes profile migrate-identity {args.old_name} {args.new_name}", err=True) + print(f"✓ Session/routing identity migrated: {args.old_name} → {args.new_name}") + + def _profile_export(args): from hermes_cli.profiles import export_profile, get_profile_export_path name = args.profile_name @@ -575,6 +590,7 @@ PROFILE_ACTIONS = { 'show': _profile_show, 'alias': _profile_alias, 'rename': _profile_rename, + 'migrate-identity': _profile_migrate_identity, 'export': _profile_export, 'import': _profile_import, 'install': _profile_install, diff --git a/hermes_cli/profiles.py b/hermes_cli/profiles.py index 3cf198d06e..48fbe6ae2a 100644 --- a/hermes_cli/profiles.py +++ b/hermes_cli/profiles.py @@ -1875,30 +1875,84 @@ def rename_profile(old_name: str, new_name: str) -> Path: return new_dir -def _migrate_profile_identity(old_canon: str, new_canon: str, live_mux: bool) -> None: - """Rekey renamed-profile identity without racing a live gateway's in-memory routing index.""" +def migrate_profile_identity(old_name: str, new_name: str) -> bool: + """Retry the session/routing identity migration of a rename that already completed. + + ``rename_profile`` runs the migration itself; this is the standalone retry behind + ``hermes profile migrate-identity `` for when that attempt failed. The rename + cannot simply be repeated — ``profiles/`` is gone — and the identity to migrate is read + from the DB rows that still name *old*, so only the new profile has to exist here. + + A live multiplexer holds the routing index in memory and therefore stays the owner of the + migration (the CLI delegates to its control verb); with no live multiplexer the durable + rewrite is safe because nothing else holds the store. Idempotent: re-running a completed + migration succeeds with nothing left to rekey. Returns True when the identity was migrated, + False when a live gateway would not do it — the caller reports that as a failure. + """ + old_canon = _canon_valid(old_name) + new_canon = _canon_valid(new_name) + if "default" in (old_canon, new_canon): + raise ValueError("Identity migration applies to named profiles only.") + if not get_profile_dir(new_canon).is_dir(): + raise _unknown_profile_error(new_canon) + return _migrate_profile_identity(old_canon, new_canon, _live_default_multiplexer()) + + +def _control_answer_failure(answer) -> str: + """Why a control-socket answer is not a success. Keeps the raw answer when the payload carries + no reason field, so a malformed or old-gateway response stays diagnosable instead of + collapsing into a generic warning.""" + if isinstance(answer, dict): + failure = answer.get("error") or answer.get("message") or answer.get("detail") + return str(failure) if failure else repr(answer) + if answer is not None: + return repr(answer) + return "no response from gateway control socket" + + +def _gateway_accepts_profile_identity_verb(root: Path) -> bool: + """True when the gateway at *root* answers a verb it has always had. Distinguishes a failed + migration verb caused by an older gateway process from one caused by no gateway at all.""" + try: + from gateway.control_socket import identify_gateway + return identify_gateway(root) is not None + except Exception: + return False + + +def _migrate_profile_identity(old_canon: str, new_canon: str, live_mux: bool) -> bool: + """Rekey renamed-profile identity without racing a live gateway's in-memory routing index. + + Returns True when the identity was migrated — by the gateway's control verb, or by this + process's durable rewrite when no gateway holds the store — and False when a live gateway did + not accept it. Never fatal to the rename, which has already happened by this point. + """ if live_mux: + from hermes_constants import get_default_hermes_root + root = get_default_hermes_root() try: - from hermes_constants import get_default_hermes_root from gateway.control_socket import migrate_gateway_profile_identity - answer = migrate_gateway_profile_identity( - get_default_hermes_root(), old_canon, new_canon) + answer = migrate_gateway_profile_identity(root, old_canon, new_canon) except Exception as exc: - answer, failure = None, f"{type(exc).__name__}: {exc}" + reason = f"{type(exc).__name__}: {exc}" else: - failure = answer.get("error") if isinstance(answer, dict) else None if isinstance(answer, dict) and answer.get("ok") is True: - return - detail = f" ({failure})" if failure else "" + return True + reason = _control_answer_failure(answer) + if answer is None and _gateway_accepts_profile_identity_verb(root): + reason += (" — the gateway is running but does not implement " + "'migrate-profile-identity' (an older process than this CLI)") print( "⚠ Profile was renamed, but the live gateway could not migrate session identity" - f"{detail}. Restart the gateway, then retry the identity migration.", + f" ({reason}). Restart the gateway, then run:\n" + f" hermes profile migrate-identity {old_canon} {new_canon}", file=sys.stderr) - return + return False from hermes_state_registry import acquire, release_or_close from hermes_constants import get_default_hermes_root root = get_default_hermes_root() + migrated = True for db_path in (root / "state.db", get_profile_dir(new_canon) / "state.db"): if not db_path.exists(): continue @@ -1907,13 +1961,16 @@ def _migrate_profile_identity(old_canon: str, new_canon: str, live_mux: bool) -> db = acquire(db_path) db.rekey_profile_state(old_canon, new_canon) except Exception as exc: - click.echo( + migrated = False + print( f"⚠ Profile was renamed, but identity migration failed for {db_path}: " - f"{type(exc).__name__}: {exc}", err=True) + f"{type(exc).__name__}: {exc}", + file=sys.stderr) finally: if db is not None: with contextlib.suppress(Exception): release_or_close(db) + return migrated # Profile env resolution (called from _apply_profile_override) diff --git a/hermes_cli/subcommands/profile.py b/hermes_cli/subcommands/profile.py index 3a11dc838a..e07405a0e2 100644 --- a/hermes_cli/subcommands/profile.py +++ b/hermes_cli/subcommands/profile.py @@ -90,6 +90,18 @@ def build_profile_parser(subparsers, *, cmd_profile: Callable) -> None: "new_name", help="New profile name (for 'default': a display name — the canonical id stays 'default')") + profile_migrate = profile_subparsers.add_parser( + "migrate-identity", + help="Retry a renamed profile's session/routing identity migration", + description="Re-run the session/routing identity migration that `hermes profile rename` " + "performs automatically. The rename has already happened when this is needed, so pass " + "the OLD and NEW names: state still keyed by the old profile name (session keys, " + "profile_name, heartbeats, routing/delivery rows) is rekeyed to the new one. Run it " + "after restarting the gateway (which reloads the routing index from the DB) or after " + "stopping it. Idempotent.") + profile_migrate.add_argument("old_name", help="Profile name before the rename") + profile_migrate.add_argument("new_name", help="Profile name after the rename") + profile_export = profile_subparsers.add_parser("export", help="Export a profile to archive") profile_export.add_argument("profile_name", help="Profile to export") profile_export.add_argument( diff --git a/skills/autonomous-ai-agents/hermes-agent/references/cli-reference.md b/skills/autonomous-ai-agents/hermes-agent/references/cli-reference.md index 10229f5bd5..855a5bf767 100644 --- a/skills/autonomous-ai-agents/hermes-agent/references/cli-reference.md +++ b/skills/autonomous-ai-agents/hermes-agent/references/cli-reference.md @@ -101,6 +101,7 @@ Webhook payloads/routes: `references/webhooks.md`. ``` hermes profile list|create NAME (--clone|--clone-all|--clone-from)|use|show|delete hermes profile rename A B | alias NAME | export NAME | import FILE +hermes profile migrate-identity A B Retry a completed rename's session/routing identity migration ``` ### Credentials & Pools diff --git a/tests/hermes_cli/test_profiles.py b/tests/hermes_cli/test_profiles.py index b8d8203228..4a5282a506 100644 --- a/tests/hermes_cli/test_profiles.py +++ b/tests/hermes_cli/test_profiles.py @@ -938,6 +938,84 @@ class TestRenameProfile: acquire.assert_not_called() assert "Restart the gateway" in capsys.readouterr().err + def test_migrate_identity_command_repairs_a_failed_live_migration(self, profile_env, capsys): + """The failed-live-migration end state must be recoverable: `hermes profile + migrate-identity ` rekeys the durable rows once no gateway holds the store, and + is idempotent (a second run has nothing left to rekey but still succeeds).""" + from hermes_cli.profile_cmd import cmd_profile + from hermes_state import SessionDB + from argparse import Namespace + tmp_path = profile_env + create_profile("oldname", no_alias=True) + old_dir = tmp_path / ".hermes" / "profiles" / "oldname" + pdb = SessionDB(old_dir / "state.db") + pdb.create_session( + "sess1", "feishu", session_key="agent:oldname:feishu:dm:chatA", + profile_name="oldname", chat_id="chatA", chat_type="dm") + pdb.close() + root_db = SessionDB(tmp_path / ".hermes" / "state.db") + root_db.save_gateway_routing_entry( + "agent:oldname:feishu:dm:chatA", + json.dumps({"session_key": "agent:oldname:feishu:dm:chatA", "session_id": "sess1", + "origin": {"platform": "feishu", "chat_id": "chatA", "profile": "oldname"}}), + scope=str(tmp_path / ".hermes" / "sessions")) + root_db.close() + + # Rename under a live multiplexer whose control verb answers nothing: the CLI warns and + # leaves the (in-memory-owned) store alone, so the rows still name the old profile. + with patch("hermes_cli.profiles.check_alias_collision", return_value="skip"), \ + patch("hermes_cli.profiles._live_default_multiplexer", return_value=True), \ + patch("hermes_cli.profiles._notify_multiplexer"), \ + patch("gateway.control_socket.migrate_gateway_profile_identity", return_value=None): + rename_profile("oldname", "newname") + assert "hermes profile migrate-identity oldname newname" in capsys.readouterr().err + + # Gateway restarted/stopped → the retry command repairs both stores. + with patch("hermes_cli.profiles._live_default_multiplexer", return_value=False): + cmd_profile(Namespace(profile_action="migrate-identity", + old_name="oldname", new_name="newname")) + assert "✓ Session/routing identity migrated" in capsys.readouterr().out + # Idempotent: nothing left to rekey, still a success. + cmd_profile(Namespace(profile_action="migrate-identity", + old_name="oldname", new_name="newname")) + + moved_db = SessionDB(tmp_path / ".hermes" / "profiles" / "newname" / "state.db") + row = moved_db._read_one( + "SELECT session_key, profile_name FROM sessions WHERE id = ?", ("sess1",)) + assert row is not None + assert row["session_key"] == "agent:newname:feishu:dm:chatA" + assert row["profile_name"] == "newname" + moved_db.close() + root_db2 = SessionDB(tmp_path / ".hermes" / "state.db") + routing = root_db2.load_gateway_routing_entries( + scope=str(tmp_path / ".hermes" / "sessions")) + assert "agent:oldname:feishu:dm:chatA" not in routing + assert "agent:newname:feishu:dm:chatA" in routing + root_db2.close() + + def test_migrate_identity_reports_the_raw_answer_and_exits_nonzero(self, profile_env, capsys): + """A live gateway that answers with something unusable must fail loudly — exit non-zero, + name the retry command, and quote the raw answer (a non-dict payload used to print a + reason-less warning). The live gateway keeps ownership: no direct DB rewrite.""" + from hermes_cli.profile_cmd import cmd_profile + from argparse import Namespace + create_profile("oldname", no_alias=True) + create_profile("newname", no_alias=True) # the rename already happened; only must exist + + with patch("hermes_cli.profiles._live_default_multiplexer", return_value=True), \ + patch("gateway.control_socket.migrate_gateway_profile_identity", + return_value="not a control answer"), \ + patch("hermes_state_registry.acquire") as acquire: + with pytest.raises(SystemExit) as excinfo: + cmd_profile(Namespace(profile_action="migrate-identity", + old_name="oldname", new_name="newname")) + + assert excinfo.value.code != 0 + err = capsys.readouterr().err + assert "not a control answer" in err + assert "hermes profile migrate-identity oldname newname" in err + acquire.assert_not_called() + # =================================================================== # TestExportImport diff --git a/website/docs/reference/profile-commands.md b/website/docs/reference/profile-commands.md index 3fa4e8c261..390af56449 100644 --- a/website/docs/reference/profile-commands.md +++ b/website/docs/reference/profile-commands.md @@ -242,6 +242,40 @@ hermes profile rename mybot assistant # ~/.local/bin/mybot → ~/.local/bin/assistant ``` +The rename also migrates the profile's persisted session/routing identity — session keys +(`agent::*`), `sessions.profile_name`, heartbeats, and routing/delivery rows — to the new +name. A live multiplexed gateway owns that migration (it holds the routing index in memory), so +when it is running the CLI delegates to it. + +## `hermes profile migrate-identity` + +```bash +hermes profile migrate-identity +``` + +Retries the identity migration of a rename that already completed. Run it if `hermes profile +rename` warned that the live gateway could not migrate session identity: restart the gateway +(it reloads the routing index from the database, so the migration lands), or stop it — with no +gateway holding the store the command performs the durable rewrite itself. + +The migration is driven by the rows that still name ``, so `profiles/` does not have +to exist; only `` is checked. Idempotent — re-running a completed migration succeeds with +nothing left to rekey. Exits non-zero when a live gateway refuses the migration, when a +database rejects the rewrite (a routing collision, a lock, or one of the two databases failing +while the other succeeds), naming the database and error. + +**Example:** + +```bash +hermes profile rename mybot assistant +# ⚠ Profile was renamed, but the live gateway could not migrate session identity (…). +# Restart the gateway, then run: +# hermes profile migrate-identity mybot assistant + +hermes profile migrate-identity mybot assistant +# ✓ Session/routing identity migrated: mybot → assistant +``` + ## `hermes profile export` ```bash diff --git a/website/docs/user-guide/profiles.md b/website/docs/user-guide/profiles.md index cd6da6ab01..3e0580c628 100644 --- a/website/docs/user-guide/profiles.md +++ b/website/docs/user-guide/profiles.md @@ -308,6 +308,7 @@ User-modified skills are never overwritten. hermes profile list # show all profiles with status hermes profile show coder # detailed info for one profile hermes profile rename coder dev-bot # rename (updates alias + service) +hermes profile migrate-identity coder dev-bot # retry a rename's identity migration hermes profile export coder # pack into coder.tar.gz (shareable; keys stripped) hermes profile import coder.tar.gz # install an archive as a new profile ``` From 91a38622dbc3df1616d7631d462516e870b5c321 Mon Sep 17 00:00:00 2001 From: KoNit-K <124019182+KoNit-K@users.noreply.github.com> Date: Wed, 16 Sep 2026 12:34:46 +0800 Subject: [PATCH 04/18] fix: guard atomic writers after profile deletion --- hermes_cli/models.py | 3 +- .../test_deleted_profile_tombstone.py | 43 ++++++++++++++++++- utils.py | 15 +++++-- 3 files changed, 56 insertions(+), 5 deletions(-) diff --git a/hermes_cli/models.py b/hermes_cli/models.py index 626e6daf41..9ac946d85c 100644 --- a/hermes_cli/models.py +++ b/hermes_cli/models.py @@ -103,8 +103,9 @@ def _write_json_cache(path: Path, data: Any, **dump_kwargs: Any) -> None: """Atomically persist a cache file (creating parents). Raises on failure — callers decide whether a failed cache write is worth logging.""" from utils import atomic_json_write + from hermes_constants import mkdir_under_hermes_home - path.parent.mkdir(parents=True, exist_ok=True) + mkdir_under_hermes_home(path.parent) atomic_json_write(path, data, **dump_kwargs) diff --git a/tests/hermes_cli/test_deleted_profile_tombstone.py b/tests/hermes_cli/test_deleted_profile_tombstone.py index 608e078ad4..e2b00a26f7 100644 --- a/tests/hermes_cli/test_deleted_profile_tombstone.py +++ b/tests/hermes_cli/test_deleted_profile_tombstone.py @@ -24,7 +24,11 @@ from hermes_cli.profiles import ( resolve_profile_env, set_active_profile, ) -from hermes_constants import named_profile_home +from hermes_constants import ( + named_profile_home, + reset_hermes_home_override, + set_hermes_home_override, +) from hermes_logging import setup_logging @@ -67,6 +71,43 @@ class TestDeletedProfileTombstone: monkeypatch.setenv("HERMES_HOME", str(profile_env / ".hermes")) assert "worker" not in _named_homes(profile_env) + def test_late_reasoning_caps_save_does_not_recreate_deleted_home(self, profile_env): + """A daemon retaining a deleted profile context must not recreate its cache tree.""" + from hermes_cli import models_reasoning_caps + + profile_dir = create_profile("worker", no_alias=True, no_skills=True) + _delete("worker") + + token = set_hermes_home_override(profile_dir) + try: + models_reasoning_caps._save_reasoning_caps_disk( + "https://example.test/v1/models", + {"example/model": {"supports_reasoning": True}}, + ) + finally: + reset_hermes_home_override(token) + + assert not profile_dir.exists() + + def test_atomic_cache_write_allows_live_profile_home(self, profile_env): + from hermes_cli.models import _write_json_cache + + profile_dir = create_profile("worker", no_alias=True, no_skills=True) + cache_path = profile_dir / "cache" / "models.json" + + _write_json_cache(cache_path, {"cached": True}) + + assert cache_path.read_text(encoding="utf-8") == '{\n "cached": true\n}' + + def test_atomic_cache_write_allows_unrelated_profiles_path(self, tmp_path): + from hermes_cli.models import _write_json_cache + + cache_path = tmp_path / "custom" / "profiles" / "cache" / "models.json" + + _write_json_cache(cache_path, {"cached": True}) + + assert cache_path.read_text(encoding="utf-8") == '{\n "cached": true\n}' + def test_empty_shell_after_delete_is_not_listed_or_served(self, profile_env): profile_dir = create_profile("worker", no_alias=True, no_skills=True) with patch("hermes_cli.profiles._cleanup_gateway_service"), patch( diff --git a/utils.py b/utils.py index 5d14cfacec..36d9651e79 100644 --- a/utils.py +++ b/utils.py @@ -240,7 +240,12 @@ def _atomic_write(path: Path, write, *, prefix: str, encoding: str = "utf-8", mo also fsyncs the parent so the rename itself is durable. The temp file is removed on any failure — ``BaseException`` on purpose, so KeyboardInterrupt / SystemExit still clean up. """ - path.parent.mkdir(parents=True, exist_ok=True) + # A profile delete leaves a tombstone beside its removed home. Background + # writers may retain that home in a context variable, so a plain mkdir here + # would resurrect the profile before the write can fail. + from hermes_constants import mkdir_under_hermes_home + + mkdir_under_hermes_home(path.parent) if mode is None and not path.exists(): mode = default_new_file_mode() original_owner = _preserve_file_owner(path) if preserve_owner else None @@ -423,7 +428,9 @@ def atomic_roundtrip_yaml_update(path: Union[str, Path], key_path: str, value: A from hermes_cli.config import _greedy_literal_match, _split_key_path path = Path(path) - path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + + mkdir_under_hermes_home(path.parent) yaml_rt, config = _roundtrip_load(path) current = config keys = _split_key_path(key_path) @@ -467,7 +474,9 @@ def atomic_roundtrip_yaml_save(path: Union[str, Path], new_state: dict) -> None: from hermes_cli.config import require_readable_config_before_write path = Path(path) - path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + + mkdir_under_hermes_home(path.parent) require_readable_config_before_write(path) yaml_rt, existing = _roundtrip_load(path) From 7f7d229f83c3a513244f33297f564d66bbbc8e27 Mon Sep 17 00:00:00 2001 From: Konstantin Khlopkov Date: Wed, 16 Sep 2026 07:46:07 +0300 Subject: [PATCH 05/18] fix(utils): atomic writers refuse to resurrect a deleted named profile home Background writers that still carry a tombstoned profile as their Hermes home (reasoning-caps warm thread, models cache, models.dev ETag, gateway lifecycle ledger, MCP OAuth tokens, memory store) re-created profiles// with a bare mkdir right before an atomic write. Route the parent-dir creation through mkdir_under_hermes_home so a deleted named profile raises FileNotFoundError and stays gone, matching the tombstone contract already enforced for logging and state. --- agent/models_dev.py | 4 +- agent/secret_sources/_cache.py | 4 +- gateway/lifecycle_ledger.py | 8 +- .../test_atomic_writers_deleted_profile.py | 158 ++++++++++++++++++ tools/mcp_oauth.py | 16 +- tools/memory_tool_store.py | 12 +- 6 files changed, 191 insertions(+), 11 deletions(-) create mode 100644 tests/utils/test_atomic_writers_deleted_profile.py diff --git a/agent/models_dev.py b/agent/models_dev.py index fd40784b9b..6b6b4d1e50 100644 --- a/agent/models_dev.py +++ b/agent/models_dev.py @@ -189,7 +189,9 @@ def _load_etag() -> str: def _save_etag(etag: str) -> None: def write() -> None: etag_path = _get_etag_path() - etag_path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + + mkdir_under_hermes_home(etag_path.parent) atomic_write_text(etag_path, etag) _quietly("save models.dev ETag", write) diff --git a/agent/secret_sources/_cache.py b/agent/secret_sources/_cache.py index fab398b2dd..7e24f7a5c1 100644 --- a/agent/secret_sources/_cache.py +++ b/agent/secret_sources/_cache.py @@ -73,7 +73,9 @@ def atomic_write_json(path: Path, payload: dict) -> None: """Secret cache entry at 0600 from creation; the containing dir is tightened to 0700 (``secure_parent_dir`` refuses ``/``, top-level dirs and the install tree). Raises ``OSError`` on failure; callers decide whether that is best-effort.""" - path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + + mkdir_under_hermes_home(path.parent) secure_parent_dir(path) atomic_json_write(path, payload, indent=None, mode=0o600) diff --git a/gateway/lifecycle_ledger.py b/gateway/lifecycle_ledger.py index a92968cbd5..8eb0364ab0 100644 --- a/gateway/lifecycle_ledger.py +++ b/gateway/lifecycle_ledger.py @@ -87,7 +87,9 @@ def _write_sentinel(payload: Dict[str, Any], home: Optional[Path]) -> None: from utils import atomic_json_write path = get_lifecycle_sentinel_path(home) - path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + + mkdir_under_hermes_home(path.parent) atomic_json_write(path, payload, indent=None) except Exception: logger.debug("Failed to write lifecycle sentinel", exc_info=True) @@ -97,7 +99,9 @@ def _append_exit_diag(record: Dict[str, Any], home: Optional[Path]) -> None: """Append a JSON line to gateway-exit-diag.log (same format as the CLI's ``_exit_diag``).""" try: path = _home_path(home, "logs", "gateway-exit-diag.log") - path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + + mkdir_under_hermes_home(path.parent) with path.open("a", encoding="utf-8") as fh: fh.write(json.dumps(record, default=str) + "\n") except OSError: diff --git a/tests/utils/test_atomic_writers_deleted_profile.py b/tests/utils/test_atomic_writers_deleted_profile.py new file mode 100644 index 0000000000..95daee405d --- /dev/null +++ b/tests/utils/test_atomic_writers_deleted_profile.py @@ -0,0 +1,158 @@ +"""Atomic writers must not resurrect a deleted named profile home. + +``hermes profile delete`` removes the tree and writes a tombstone under +``profiles/.deleted/``. Background writers that still carry the dead +profile as their Hermes home (reasoning-caps warm thread, models.dev refresh, +gateway lifecycle ledger, MCP OAuth token writes, memory store mutations) used +to re-create ``profiles//`` with a bare ``mkdir(parents=True)`` right +before an atomic write — the exact resurrection class the tombstone guard +closed for logging and state. These tests lock the same contract for the +writers themselves: a tombstoned home raises ``FileNotFoundError`` and leaves +nothing on disk, while unrelated paths with a ``profiles`` path segment keep +working. +""" + +from __future__ import annotations + +import json +from pathlib import Path + +import pytest + +from hermes_constants import ( + mark_named_profile_deleted, + named_profile_home, + set_hermes_home_override, +) +from utils import atomic_json_write, atomic_write_text + + +def _tombstoned_profile(tmp_path: Path) -> Path: + """A real ``/profiles/`` home that has just been deleted.""" + (tmp_path / "config.yaml").write_text("{}\n", encoding="utf-8") + profile = tmp_path / "profiles" / "p1" + profile.mkdir(parents=True) + mark_named_profile_deleted(profile) + import shutil + + shutil.rmtree(profile) + assert named_profile_home(profile) is not None + assert not profile.exists() + return profile + + +class TestAtomicWritersRefuseDeletedProfileHome: + def test_atomic_json_write_does_not_recreate_home(self, tmp_path): + profile = _tombstoned_profile(tmp_path) + with pytest.raises( + FileNotFoundError, match="Named profile home does not exist" + ): + atomic_json_write(profile / "cache" / "reasoning_caps.json", {"m": {}}) + assert not profile.exists() + + def test_atomic_write_text_does_not_recreate_home(self, tmp_path): + profile = _tombstoned_profile(tmp_path) + with pytest.raises( + FileNotFoundError, match="Named profile home does not exist" + ): + atomic_write_text(profile / "cache" / "models-dev-etag", "etag") + assert not profile.exists() + + def test_late_reasoning_caps_save_after_delete(self, tmp_path): + from hermes_cli import models_reasoning_caps + + profile = _tombstoned_profile(tmp_path) + token = set_hermes_home_override(profile) + try: + models_reasoning_caps._save_reasoning_caps_disk( + "https://example/v1/models", {"m": {"supports_reasoning": True}} + ) + finally: + from hermes_constants import reset_hermes_home_override + + reset_hermes_home_override(token) + assert not (profile / "cache" / "reasoning_caps.json").exists() + assert not profile.exists() + + def test_late_models_cache_save_after_delete(self, tmp_path): + from hermes_cli.models import _write_json_cache + + profile = _tombstoned_profile(tmp_path) + token = set_hermes_home_override(profile) + try: + with pytest.raises( + FileNotFoundError, match="Named profile home does not exist" + ): + _write_json_cache( + profile / "cache" / "reasoning_caps.json", + {"m": {}}, + indent=0, + separators=(",", ":"), + ) + finally: + from hermes_constants import reset_hermes_home_override + + reset_hermes_home_override(token) + assert not profile.exists() + + def test_lifecycle_sentinel_write_after_delete(self, tmp_path): + from gateway.lifecycle_ledger import _write_sentinel + + profile = _tombstoned_profile(tmp_path) + _write_sentinel({"reason": "clean-exit"}, profile) + assert not profile.exists() + + def test_oauth_token_write_after_delete(self, tmp_path): + from tools.mcp_oauth import _write_json + + profile = _tombstoned_profile(tmp_path) + with pytest.raises( + FileNotFoundError, match="Named profile home does not exist" + ): + _write_json(profile / "mcp-oauth" / "tokens.json", {"access_token": "x"}) + assert not profile.exists() + + def test_memory_store_add_after_delete(self, tmp_path): + from tools.memory_tool_store import MemoryStore + + profile = _tombstoned_profile(tmp_path) + token = set_hermes_home_override(profile) + try: + store = MemoryStore(memory_char_limit=100, user_char_limit=100) + with pytest.raises( + FileNotFoundError, match="Named profile home does not exist" + ): + store.add("memory", "entry") + finally: + from hermes_constants import reset_hermes_home_override + + reset_hermes_home_override(token) + assert not profile.exists() + + def test_roundtrip_yaml_update_does_not_recreate_home(self, tmp_path): + from utils import atomic_roundtrip_yaml_update + + profile = _tombstoned_profile(tmp_path) + with pytest.raises( + FileNotFoundError, match="Named profile home does not exist" + ): + atomic_roundtrip_yaml_update(profile / "config.yaml", "model", "glm-5.3") + assert not profile.exists() + + +class TestUnrelatedProfilesPathsStillWrite: + def test_custom_home_with_profiles_segment_writes(self, tmp_path): + custom_home = tmp_path / "srv" / "profiles" / "buildcache" + atomic_json_write(custom_home / "cache" / "blob.json", {"a": 1}) + assert json.loads( + (custom_home / "cache" / "blob.json").read_text(encoding="utf-8") + ) == {"a": 1} + + def test_default_home_cache_write(self, tmp_path): + home = tmp_path / ".hermes" + atomic_write_text(home / "cache" / "etag", "v1") + assert (home / "cache" / "etag").read_text(encoding="utf-8") == "v1" + + def test_plain_tmp_path_write(self, tmp_path): + atomic_json_write(tmp_path / "plain" / "data.json", [1, 2]) + assert (tmp_path / "plain" / "data.json").exists() diff --git a/tools/mcp_oauth.py b/tools/mcp_oauth.py index 6e17a8e821..885ae40c48 100644 --- a/tools/mcp_oauth.py +++ b/tools/mcp_oauth.py @@ -108,7 +108,9 @@ async def acquire_refresh_fence(path: "Path", *, timeout: float = _REFRESH_FENCE """ lock_path = _refresh_lock_path(path) try: - lock_path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + + mkdir_under_hermes_home(lock_path.parent) secure_parent_dir(lock_path) fd = os.open(lock_path, os.O_RDWR | os.O_CREAT, 0o600) except OSError as exc: @@ -386,7 +388,9 @@ def _read_json(path: Path) -> dict | None: def _write_json(path: Path, data: dict) -> None: """OAuth tokens/client info at 0600 from creation, parent tightened to 0700 (``secure_parent_dir`` refuses ``/``, top-level dirs and the install tree — #25821, #93050).""" - path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + + mkdir_under_hermes_home(path.parent) secure_parent_dir(path) atomic_json_write(path, data, mode=0o600, default=str) @@ -555,7 +559,9 @@ class HermesTokenStorage: the refused client_id. Cleared by ``remove()`` so a fixed document gets a retry.""" path = self._cimd_rejected_path() try: - path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + + mkdir_under_hermes_home(path.parent) path.touch() except OSError as exc: # non-fatal — worst case we retry CIMD later logger.debug("Could not record CIMD rejection at %s: %s", path, exc) @@ -589,7 +595,9 @@ class HermesTokenStorage: if not snapshot: return token_dir = _get_token_dir(self._hermes_home) - token_dir.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + + mkdir_under_hermes_home(token_dir) for fname, data in snapshot.items(): try: fd = os.open(str(token_dir / fname), os.O_WRONLY | os.O_CREAT | os.O_TRUNC, stat.S_IRUSR | stat.S_IWUSR) diff --git a/tools/memory_tool_store.py b/tools/memory_tool_store.py index 7098f91899..0bca155602 100644 --- a/tools/memory_tool_store.py +++ b/tools/memory_tool_store.py @@ -127,7 +127,9 @@ class MemoryStore: for target in ("memory", "user"): path = self._path_for(target) - path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + + mkdir_under_hermes_home(path.parent) # Deduplicate (order-preserving, first occurrence wins). entries = list(dict.fromkeys(self._read_file(path))) self._set_entries(target, entries) @@ -148,7 +150,9 @@ class MemoryStore: from tools import memory_tool as _mt # fcntl/msvcrt live (and are patched) there fcntl, msvcrt = _mt.fcntl, _mt.msvcrt lock_path = path.with_suffix(path.suffix + ".lock") - lock_path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + + mkdir_under_hermes_home(lock_path.parent) if fcntl is None and msvcrt is None: yield return @@ -230,7 +234,9 @@ class MemoryStore: if isinstance(result, dict): return result self._set_entries(target, result[0]) - path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + + mkdir_under_hermes_home(path.parent) self._write_file(path, result[0]) return self._success_response(target, result[1]) From 5d97d5ed6d53725e8b23b7c2c974c77b56a888eb Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Tue, 15 Sep 2026 22:50:13 -0700 Subject: [PATCH 06/18] refactor(profiles): move rename identity migration off the facades; trim tests to invariants - hermes_cli/profiles.py was already past the 2,000-line gate; the rename identity migration (migrate_profile_identity, _migrate_profile_identity, control-answer helpers) now lives in hermes_cli/profile_identity.py, imported late from rename_profile and the profile subcommand. - The migrate-profile-identity control-verb handler moves out of gateway/run.py into gateway/run_profile_reconcile.py (migrate_profile_identity_verb), beside the other hot-serve control-verb logic. - tests/utils/ is not a mirrored source dir: the atomic-writer tests move to tests/test_utils_atomic_writers_deleted_profile.py (root-module test placement). - Drop duplicate no-op/idempotent/raw-answer tests so each fix carries invariant tests only. Behaviour unchanged; authored commits from @xielevi, @KoNit-K and @kokhlo are preserved. --- gateway/run.py | 36 +----- gateway/run_profile_reconcile.py | 42 +++++++ hermes_cli/profile_cmd.py | 2 +- hermes_cli/profile_identity.py | 119 ++++++++++++++++++ hermes_cli/profiles.py | 99 +-------------- tests/gateway/test_rekey_profile_routing.py | 10 -- tests/hermes_cli/test_profiles.py | 27 ---- .../hermes_state/test_rekey_profile_state.py | 18 --- ...t_utils_atomic_writers_deleted_profile.py} | 33 ----- 9 files changed, 165 insertions(+), 221 deletions(-) create mode 100644 hermes_cli/profile_identity.py rename tests/{utils/test_atomic_writers_deleted_profile.py => test_utils_atomic_writers_deleted_profile.py} (77%) diff --git a/gateway/run.py b/gateway/run.py index 26a3a67f59..a072dac7dd 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -5071,6 +5071,7 @@ async def _start_gateway_start_control_socket(runner): # failure only means consumers fall back to the process-scan/state-file layer, exactly as before # this feature. See #92091. from gateway.control_socket import GatewayControlServer + from gateway.run_profile_reconcile import migrate_profile_identity_verb # pause-for-update: the updater asks us to drain + exit (freeing venv handles) vs. a tree-kill # (same path as SIGUSR1). Handler runs on the socket executor thread, so marshal onto the loop. # pause-for-update (#92091 step 2): the updater asks this gateway to drain in-flight turns and exit @@ -5116,43 +5117,10 @@ async def _start_gateway_start_control_socket(runner): except concurrent.futures.TimeoutError: return {"multiplex": True, "pending": True, "served_profiles": runner.served_profile_names()} - def _migrate_profile_identity_handler(params: dict) -> dict: - """Migrate both durable stores and the routing index owned by this live gateway.""" - old, new = str(params.get("old") or "").strip(), str(params.get("new") or "").strip() - if not old or not new or old == new: - return {"ok": False, "error": "old/new required and must differ"} - store = getattr(runner, "session_store", None) - if store is None: - return {"ok": False, "error": "live gateway has no session store"} - acquired = [] - try: - from hermes_state_registry import acquire, release_or_close - db_counts: dict[str, dict[str, int]] = {} - routing_db = getattr(store, "_routing_db", None) - if routing_db is not None and hasattr(routing_db, "rekey_profile_state"): - db_counts["routing"] = routing_db.rekey_profile_state(old, new) - routing_home = getattr(store, "_routing_home", None) - profile_path = Path(routing_home) / "profiles" / new / "state.db" if routing_home else None - if profile_path is not None and profile_path.exists(): - profile_db = acquire(profile_path) - acquired.append(profile_db) - db_counts["profile"] = profile_db.rekey_profile_state(old, new) - rekeyed = store.rekey_profile_routing(old, new) - return {"ok": True, "rekeyed": rekeyed, "db": db_counts} - except Exception as exc: - logger.warning("Profile identity migration failed for %r->%r: %s", old, new, exc) - return {"ok": False, "error": f"{type(exc).__name__}: {exc}"} - finally: - for db in acquired: - try: - release_or_close(db) - except Exception: - logger.debug("Failed to release renamed profile state DB", exc_info=True) - _control_server = GatewayControlServer( verb_handlers={"pause-for-update": _pause_for_update_handler, "rescan-profiles": _rescan_profiles_handler, - "migrate-profile-identity": _migrate_profile_identity_handler}) + "migrate-profile-identity": migrate_profile_identity_verb(runner)}) if not await _control_server.start(): _control_server = None else: diff --git a/gateway/run_profile_reconcile.py b/gateway/run_profile_reconcile.py index 4aa4ed975f..c127770f2b 100644 --- a/gateway/run_profile_reconcile.py +++ b/gateway/run_profile_reconcile.py @@ -262,3 +262,45 @@ def _mcp_config_reconciler(runner=None): _reconcile_current(str(profile_name)) return _tick + + +def migrate_profile_identity_verb(runner): + """Build the ``migrate-profile-identity`` control-verb handler for ``hermes profile rename`` + (#111926). The live multiplexer owns the routing index in memory and writes it back + periodically, so a CLI-side rewrite of ``agent::*`` would be clobbered on the next save; + the CLI therefore asks this process to rekey both durable stores AND ``SessionStore._entries``. + Runs on the control-socket executor thread; ``rekey_profile_routing`` takes the store lock.""" + + def _handler(params: dict) -> dict: + old, new = str(params.get("old") or "").strip(), str(params.get("new") or "").strip() + if not old or not new or old == new: + return {"ok": False, "error": "old/new required and must differ"} + store = getattr(runner, "session_store", None) + if store is None: + return {"ok": False, "error": "live gateway has no session store"} + acquired = [] + try: + from hermes_state_registry import acquire, release_or_close + db_counts: Dict[str, Dict[str, int]] = {} + routing_db = getattr(store, "_routing_db", None) + if routing_db is not None and hasattr(routing_db, "rekey_profile_state"): + db_counts["routing"] = routing_db.rekey_profile_state(old, new) + routing_home = getattr(store, "_routing_home", None) + profile_path = Path(routing_home) / "profiles" / new / "state.db" if routing_home else None + if profile_path is not None and profile_path.exists(): + profile_db = acquire(profile_path) + acquired.append(profile_db) + db_counts["profile"] = profile_db.rekey_profile_state(old, new) + rekeyed = store.rekey_profile_routing(old, new) + return {"ok": True, "rekeyed": rekeyed, "db": db_counts} + except Exception as exc: + logger.warning("Profile identity migration failed for %r->%r: %s", old, new, exc) + return {"ok": False, "error": f"{type(exc).__name__}: {exc}"} + finally: + for db in acquired: + try: + release_or_close(db) + except Exception: + logger.debug("Failed to release renamed profile state DB", exc_info=True) + + return _handler diff --git a/hermes_cli/profile_cmd.py b/hermes_cli/profile_cmd.py index 2d355fe721..f20165a35f 100644 --- a/hermes_cli/profile_cmd.py +++ b/hermes_cli/profile_cmd.py @@ -440,7 +440,7 @@ def _profile_migrate_identity(args): """Retry the identity migration of a rename that already completed. Exits non-zero when a live gateway would not migrate (it still owns the routing index in memory), or when a database rejected the rewrite (collision, lock, partial failure).""" - from hermes_cli.profiles import migrate_profile_identity + from hermes_cli.profile_identity import migrate_profile_identity try: migrated = migrate_profile_identity(args.old_name, args.new_name) except (ValueError, FileNotFoundError) as e: diff --git a/hermes_cli/profile_identity.py b/hermes_cli/profile_identity.py new file mode 100644 index 0000000000..b70e81ee17 --- /dev/null +++ b/hermes_cli/profile_identity.py @@ -0,0 +1,119 @@ +"""Rekey a renamed profile's session/routing identity (#111926). + +``rename_profile`` moves ``profiles//`` to ``profiles//`` so row DATA travels with the +directory, but the profile name is also baked into keys and values the move never touches: +``agent::*`` session-key namespaces (routing index + the profile's own ``sessions`` rows), +``sessions.profile_name``, ``gateway_heartbeats.profile`` and ``delivery_obligations``. Left alone, +every inbound event on a chat keyed to the old name logs ``Profile 'old' does not exist`` and +falls back to the global home, and renamed sessions drop out of the Desktop sidebar. + +Ownership decides who rewrites: a live multiplexer holds the routing index in memory +(``SessionStore._entries``) and writes it back periodically, so a CLI-side DB rewrite would be +clobbered on its next save — the CLI delegates to the ``migrate-profile-identity`` control verb. +With no live multiplexer nothing else holds the store and the durable rewrite is safe here. +""" +from __future__ import annotations + +import contextlib +import sys +from pathlib import Path + + +def migrate_profile_identity(old_name: str, new_name: str) -> bool: + """Retry the session/routing identity migration of a rename that already completed. + + ``rename_profile`` runs the migration itself; this is the standalone retry behind + ``hermes profile migrate-identity `` for when that attempt failed. The rename + cannot simply be repeated — ``profiles/`` is gone — and the identity to migrate is read + from the DB rows that still name *old*, so only the new profile has to exist here. + + A live multiplexer holds the routing index in memory and therefore stays the owner of the + migration (the CLI delegates to its control verb); with no live multiplexer the durable + rewrite is safe because nothing else holds the store. Idempotent: re-running a completed + migration succeeds with nothing left to rekey. Returns True when the identity was migrated, + False when a live gateway would not do it — the caller reports that as a failure. + """ + from hermes_cli.profiles import _canon_valid, _live_default_multiplexer, _unknown_profile_error, get_profile_dir + old_canon = _canon_valid(old_name) + new_canon = _canon_valid(new_name) + if "default" in (old_canon, new_canon): + raise ValueError("Identity migration applies to named profiles only.") + if not get_profile_dir(new_canon).is_dir(): + raise _unknown_profile_error(new_canon) + return _migrate_profile_identity(old_canon, new_canon, _live_default_multiplexer()) + + +def _control_answer_failure(answer) -> str: + """Why a control-socket answer is not a success. Keeps the raw answer when the payload carries + no reason field, so a malformed or old-gateway response stays diagnosable instead of + collapsing into a generic warning.""" + if isinstance(answer, dict): + failure = answer.get("error") or answer.get("message") or answer.get("detail") + return str(failure) if failure else repr(answer) + if answer is not None: + return repr(answer) + return "no response from gateway control socket" + + +def _gateway_accepts_profile_identity_verb(root: Path) -> bool: + """True when the gateway at *root* answers a verb it has always had. Distinguishes a failed + migration verb caused by an older gateway process from one caused by no gateway at all.""" + try: + from gateway.control_socket import identify_gateway + return identify_gateway(root) is not None + except Exception: + return False + + +def _migrate_profile_identity(old_canon: str, new_canon: str, live_mux: bool) -> bool: + """Rekey renamed-profile identity without racing a live gateway's in-memory routing index. + + Returns True when the identity was migrated — by the gateway's control verb, or by this + process's durable rewrite when no gateway holds the store — and False when a live gateway did + not accept it. Never fatal to the rename, which has already happened by this point. + """ + if live_mux: + from hermes_constants import get_default_hermes_root + root = get_default_hermes_root() + try: + from gateway.control_socket import migrate_gateway_profile_identity + answer = migrate_gateway_profile_identity(root, old_canon, new_canon) + except Exception as exc: + reason = f"{type(exc).__name__}: {exc}" + else: + if isinstance(answer, dict) and answer.get("ok") is True: + return True + reason = _control_answer_failure(answer) + if answer is None and _gateway_accepts_profile_identity_verb(root): + reason += (" — the gateway is running but does not implement " + "'migrate-profile-identity' (an older process than this CLI)") + print( + "⚠ Profile was renamed, but the live gateway could not migrate session identity" + f" ({reason}). Restart the gateway, then run:\n" + f" hermes profile migrate-identity {old_canon} {new_canon}", + file=sys.stderr) + return False + + from hermes_cli.profiles import get_profile_dir + from hermes_state_registry import acquire, release_or_close + from hermes_constants import get_default_hermes_root + root = get_default_hermes_root() + migrated = True + for db_path in (root / "state.db", get_profile_dir(new_canon) / "state.db"): + if not db_path.exists(): + continue + db = None + try: + db = acquire(db_path) + db.rekey_profile_state(old_canon, new_canon) + except Exception as exc: + migrated = False + print( + f"⚠ Profile was renamed, but identity migration failed for {db_path}: " + f"{type(exc).__name__}: {exc}", + file=sys.stderr) + finally: + if db is not None: + with contextlib.suppress(Exception): + release_or_close(db) + return migrated diff --git a/hermes_cli/profiles.py b/hermes_cli/profiles.py index 48fbe6ae2a..a3ce6412c4 100644 --- a/hermes_cli/profiles.py +++ b/hermes_cli/profiles.py @@ -1867,6 +1867,7 @@ def rename_profile(old_name: str, new_name: str) -> Path: # 6. Migrate profile-name-keyed session/routing state (session keys, profile_name, heartbeats, # delivery + routing index) from the old name to the new one. A stale ``agent::*`` routing # key otherwise resolves to a profile that no longer exists on every inbound event. + from hermes_cli.profile_identity import _migrate_profile_identity _migrate_profile_identity(old_canon, new_canon, live_mux) # 7. Hot-serve the renamed profile now (mirrors create; a missed signal only delays it). @@ -1875,104 +1876,6 @@ def rename_profile(old_name: str, new_name: str) -> Path: return new_dir -def migrate_profile_identity(old_name: str, new_name: str) -> bool: - """Retry the session/routing identity migration of a rename that already completed. - - ``rename_profile`` runs the migration itself; this is the standalone retry behind - ``hermes profile migrate-identity `` for when that attempt failed. The rename - cannot simply be repeated — ``profiles/`` is gone — and the identity to migrate is read - from the DB rows that still name *old*, so only the new profile has to exist here. - - A live multiplexer holds the routing index in memory and therefore stays the owner of the - migration (the CLI delegates to its control verb); with no live multiplexer the durable - rewrite is safe because nothing else holds the store. Idempotent: re-running a completed - migration succeeds with nothing left to rekey. Returns True when the identity was migrated, - False when a live gateway would not do it — the caller reports that as a failure. - """ - old_canon = _canon_valid(old_name) - new_canon = _canon_valid(new_name) - if "default" in (old_canon, new_canon): - raise ValueError("Identity migration applies to named profiles only.") - if not get_profile_dir(new_canon).is_dir(): - raise _unknown_profile_error(new_canon) - return _migrate_profile_identity(old_canon, new_canon, _live_default_multiplexer()) - - -def _control_answer_failure(answer) -> str: - """Why a control-socket answer is not a success. Keeps the raw answer when the payload carries - no reason field, so a malformed or old-gateway response stays diagnosable instead of - collapsing into a generic warning.""" - if isinstance(answer, dict): - failure = answer.get("error") or answer.get("message") or answer.get("detail") - return str(failure) if failure else repr(answer) - if answer is not None: - return repr(answer) - return "no response from gateway control socket" - - -def _gateway_accepts_profile_identity_verb(root: Path) -> bool: - """True when the gateway at *root* answers a verb it has always had. Distinguishes a failed - migration verb caused by an older gateway process from one caused by no gateway at all.""" - try: - from gateway.control_socket import identify_gateway - return identify_gateway(root) is not None - except Exception: - return False - - -def _migrate_profile_identity(old_canon: str, new_canon: str, live_mux: bool) -> bool: - """Rekey renamed-profile identity without racing a live gateway's in-memory routing index. - - Returns True when the identity was migrated — by the gateway's control verb, or by this - process's durable rewrite when no gateway holds the store — and False when a live gateway did - not accept it. Never fatal to the rename, which has already happened by this point. - """ - if live_mux: - from hermes_constants import get_default_hermes_root - root = get_default_hermes_root() - try: - from gateway.control_socket import migrate_gateway_profile_identity - answer = migrate_gateway_profile_identity(root, old_canon, new_canon) - except Exception as exc: - reason = f"{type(exc).__name__}: {exc}" - else: - if isinstance(answer, dict) and answer.get("ok") is True: - return True - reason = _control_answer_failure(answer) - if answer is None and _gateway_accepts_profile_identity_verb(root): - reason += (" — the gateway is running but does not implement " - "'migrate-profile-identity' (an older process than this CLI)") - print( - "⚠ Profile was renamed, but the live gateway could not migrate session identity" - f" ({reason}). Restart the gateway, then run:\n" - f" hermes profile migrate-identity {old_canon} {new_canon}", - file=sys.stderr) - return False - - from hermes_state_registry import acquire, release_or_close - from hermes_constants import get_default_hermes_root - root = get_default_hermes_root() - migrated = True - for db_path in (root / "state.db", get_profile_dir(new_canon) / "state.db"): - if not db_path.exists(): - continue - db = None - try: - db = acquire(db_path) - db.rekey_profile_state(old_canon, new_canon) - except Exception as exc: - migrated = False - print( - f"⚠ Profile was renamed, but identity migration failed for {db_path}: " - f"{type(exc).__name__}: {exc}", - file=sys.stderr) - finally: - if db is not None: - with contextlib.suppress(Exception): - release_or_close(db) - return migrated - - # Profile env resolution (called from _apply_profile_override) def resolve_profile_env(profile_name: str) -> str: diff --git a/tests/gateway/test_rekey_profile_routing.py b/tests/gateway/test_rekey_profile_routing.py index daef3d6d97..f57944bb7b 100644 --- a/tests/gateway/test_rekey_profile_routing.py +++ b/tests/gateway/test_rekey_profile_routing.py @@ -51,16 +51,6 @@ def test_rekeys_old_namespace_and_origin_profile(tmp_path): assert store._entries["agent:keepme:feishu:dm:chatB"].origin.profile == "keepme" -def test_noop_for_equal_or_empty_names(tmp_path): - store = _make_store(tmp_path) - with store._lock: - store._entries["agent:oldname:feishu:dm:chatA"] = _entry( - "agent:oldname:feishu:dm:chatA", "chatA", "oldname") - assert store.rekey_profile_routing("x", "x") == 0 - assert store.rekey_profile_routing("", "y") == 0 - assert "agent:oldname:feishu:dm:chatA" in store._entries - - def test_does_not_overwrite_existing_new_namespace_key(tmp_path): store = _make_store(tmp_path) with store._lock: diff --git a/tests/hermes_cli/test_profiles.py b/tests/hermes_cli/test_profiles.py index 4a5282a506..b2ba3f30f3 100644 --- a/tests/hermes_cli/test_profiles.py +++ b/tests/hermes_cli/test_profiles.py @@ -993,33 +993,6 @@ class TestRenameProfile: assert "agent:newname:feishu:dm:chatA" in routing root_db2.close() - def test_migrate_identity_reports_the_raw_answer_and_exits_nonzero(self, profile_env, capsys): - """A live gateway that answers with something unusable must fail loudly — exit non-zero, - name the retry command, and quote the raw answer (a non-dict payload used to print a - reason-less warning). The live gateway keeps ownership: no direct DB rewrite.""" - from hermes_cli.profile_cmd import cmd_profile - from argparse import Namespace - create_profile("oldname", no_alias=True) - create_profile("newname", no_alias=True) # the rename already happened; only must exist - - with patch("hermes_cli.profiles._live_default_multiplexer", return_value=True), \ - patch("gateway.control_socket.migrate_gateway_profile_identity", - return_value="not a control answer"), \ - patch("hermes_state_registry.acquire") as acquire: - with pytest.raises(SystemExit) as excinfo: - cmd_profile(Namespace(profile_action="migrate-identity", - old_name="oldname", new_name="newname")) - - assert excinfo.value.code != 0 - err = capsys.readouterr().err - assert "not a control answer" in err - assert "hermes profile migrate-identity oldname newname" in err - acquire.assert_not_called() - - -# =================================================================== -# TestExportImport -# =================================================================== class TestExportImport: """Tests for export_profile() / import_profile().""" diff --git a/tests/hermes_state/test_rekey_profile_state.py b/tests/hermes_state/test_rekey_profile_state.py index 307338fbd2..92ba91a1fc 100644 --- a/tests/hermes_state/test_rekey_profile_state.py +++ b/tests/hermes_state/test_rekey_profile_state.py @@ -100,23 +100,6 @@ class TestRekeyProfileState: assert ob["session_key"] == "agent:newname:feishu:dm:chatA" assert ob["adapter_profile"] == "newname" - def test_noop_when_names_equal_or_empty(self, db): - assert db.rekey_profile_state("x", "x") == {} - assert db.rekey_profile_state("", "y") == {} - assert db.rekey_profile_state("x", "") == {} - - def test_idempotent(self, db): - db.create_session( - "sess_old", "feishu", session_key="agent:oldname:feishu:dm:chatA", - profile_name="oldname", chat_id="chatA", chat_type="dm", - ) - first = db.rekey_profile_state("oldname", "newname") - assert first["sessions_session_key"] == 1 - second = db.rekey_profile_state("oldname", "newname") - # Nothing left under the old name. - assert second["sessions_session_key"] == 0 - assert second["sessions_profile_name"] == 0 - def test_underscore_in_profile_name_is_not_a_like_wildcard(self, db): db.create_session( "literal", "telegram", session_key="agent:foo_bar:telegram:dm:a", @@ -154,4 +137,3 @@ class TestRekeyProfileState: "WHERE chat_id = ? AND thread_id = ?", ("chatA", "threadA")) assert binding["profile_name"] == "newname" assert binding["session_key"] == "agent:newname:telegram:dm:chatA" - diff --git a/tests/utils/test_atomic_writers_deleted_profile.py b/tests/test_utils_atomic_writers_deleted_profile.py similarity index 77% rename from tests/utils/test_atomic_writers_deleted_profile.py rename to tests/test_utils_atomic_writers_deleted_profile.py index 95daee405d..7da2d29e8b 100644 --- a/tests/utils/test_atomic_writers_deleted_profile.py +++ b/tests/test_utils_atomic_writers_deleted_profile.py @@ -50,30 +50,6 @@ class TestAtomicWritersRefuseDeletedProfileHome: atomic_json_write(profile / "cache" / "reasoning_caps.json", {"m": {}}) assert not profile.exists() - def test_atomic_write_text_does_not_recreate_home(self, tmp_path): - profile = _tombstoned_profile(tmp_path) - with pytest.raises( - FileNotFoundError, match="Named profile home does not exist" - ): - atomic_write_text(profile / "cache" / "models-dev-etag", "etag") - assert not profile.exists() - - def test_late_reasoning_caps_save_after_delete(self, tmp_path): - from hermes_cli import models_reasoning_caps - - profile = _tombstoned_profile(tmp_path) - token = set_hermes_home_override(profile) - try: - models_reasoning_caps._save_reasoning_caps_disk( - "https://example/v1/models", {"m": {"supports_reasoning": True}} - ) - finally: - from hermes_constants import reset_hermes_home_override - - reset_hermes_home_override(token) - assert not (profile / "cache" / "reasoning_caps.json").exists() - assert not profile.exists() - def test_late_models_cache_save_after_delete(self, tmp_path): from hermes_cli.models import _write_json_cache @@ -147,12 +123,3 @@ class TestUnrelatedProfilesPathsStillWrite: assert json.loads( (custom_home / "cache" / "blob.json").read_text(encoding="utf-8") ) == {"a": 1} - - def test_default_home_cache_write(self, tmp_path): - home = tmp_path / ".hermes" - atomic_write_text(home / "cache" / "etag", "v1") - assert (home / "cache" / "etag").read_text(encoding="utf-8") == "v1" - - def test_plain_tmp_path_write(self, tmp_path): - atomic_json_write(tmp_path / "plain" / "data.json", [1, 2]) - assert (tmp_path / "plain" / "data.json").exists() From 9085ef967cc275adf9c4bd0ba10be2d1ab1809c1 Mon Sep 17 00:00:00 2001 From: Sora-bluesky Date: Wed, 16 Sep 2026 13:26:22 +0900 Subject: [PATCH 07/18] fix(profiles): sweep the remaining pre-write mkdirs under the deleted-profile guard A long-lived serve process keeps a deleted profile as the context home of threads that outlive the delete. A bare `mkdir(parents=True)` right before an atomic write brings `profiles//` back after `hermes profile delete` has written the tombstone and removed the tree. The writers in `utils` and the seven callers named in #112592 are guarded by the preceding commits; this one applies the same `mkdir_under_hermes_home` idiom to the other pre-write directory creations found by the same mechanical rule (auth, personality, plugin catalog, skills sync, tool discovery cache, platform adapters, memory plugins, local runtime supervisor, process identity, breadcrumbs). The two sites that pass `mode=` keep their mkdir behind `assert_named_profile_home_live`. The guard is a no-op unless the target has a provable `profiles/` ancestor. Salvaged from #112596 (30-file sweep) on top of #112594 / #112601; the overlapping files were resolved to the already-landed versions. --- cli.py | 3 ++- gateway/shutdown_flush.py | 2 ++ hermes_cli/auth.py | 3 ++- hermes_cli/auth_oauth_grants.py | 3 ++- hermes_cli/install_identity.py | 3 ++- hermes_cli/local_runtime/supervisor.py | 3 ++- hermes_cli/personality.py | 3 ++- hermes_cli/plugin_catalog.py | 4 +++- hermes_cli/process_identity.py | 3 ++- hermes_cli/terminal_breadcrumbs.py | 3 ++- hermes_cli/web_routers/memory_providers.py | 3 ++- plugins/memory/honcho/cli.py | 3 ++- plugins/memory/openviking/__init__.py | 3 ++- plugins/memory/openviking/_setup.py | 3 ++- plugins/platforms/feishu/adapter.py | 3 ++- plugins/platforms/google_chat/oauth.py | 3 ++- tools/bot_relay.py | 3 ++- tools/process_registry_results.py | 2 ++ tools/registry.py | 3 ++- tools/skill_manager_tool.py | 6 ++++-- tools/skills_sync.py | 3 ++- 21 files changed, 45 insertions(+), 20 deletions(-) diff --git a/cli.py b/cli.py index 9a9b6ce0bb..aa037e649a 100644 --- a/cli.py +++ b/cli.py @@ -2399,7 +2399,8 @@ def save_config_value(key_path: str, value: any) -> bool: config_path = get_hermes_home() / 'config.yaml' try: - config_path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + mkdir_under_hermes_home(config_path.parent) from utils import atomic_roundtrip_yaml_update atomic_roundtrip_yaml_update(config_path, key_path, value) try: # owner-only: config files contain API keys diff --git a/gateway/shutdown_flush.py b/gateway/shutdown_flush.py index 7c9d141a2a..b0af857d89 100644 --- a/gateway/shutdown_flush.py +++ b/gateway/shutdown_flush.py @@ -35,6 +35,8 @@ def _get_flush_dir(): """Return the pending-messages flush directory under the active HERMES_HOME.""" from hermes_constants import get_hermes_home flush_dir = get_hermes_home() / "pending_messages" + from hermes_constants import assert_named_profile_home_live + assert_named_profile_home_live(flush_dir) flush_dir.mkdir(parents=True, exist_ok=True, mode=0o700) if os.name == "posix": os.chmod(flush_dir, 0o700) diff --git a/hermes_cli/auth.py b/hermes_cli/auth.py index 79522c3c3d..3931be3395 100644 --- a/hermes_cli/auth.py +++ b/hermes_cli/auth.py @@ -698,7 +698,8 @@ def _load_auth_store(auth_file: Optional[Path] = None) -> Dict[str, Any]: def _save_private_json(target: Path, data: Any, *, fsync_dir: bool = False, **dump_kwargs: Any) -> None: """0600 credential JSON under a 0700 parent (``secure_parent_dir`` refuses ``/``, top-level dirs and the install tree). ``atomic_json_write`` creates the temp file 0600 before any byte lands.""" - target.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + mkdir_under_hermes_home(target.parent) secure_parent_dir(target) atomic_json_write(target, data, mode=0o600, fsync_dir=fsync_dir, **dump_kwargs) diff --git a/hermes_cli/auth_oauth_grants.py b/hermes_cli/auth_oauth_grants.py index 87a60e1fd3..11913ef471 100644 --- a/hermes_cli/auth_oauth_grants.py +++ b/hermes_cli/auth_oauth_grants.py @@ -207,7 +207,8 @@ def _persist_oauth_heal_clean_mark(provider_id: str, fingerprint: tuple) -> None if marks.get(provider_id) == new_mark: return # already recorded; skip the rewrite marks[provider_id] = new_mark - path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + mkdir_under_hermes_home(path.parent) # 0o600 like the MCP schema cache: this names credential-store paths. atomic_json_write(path, marks, mode=0o600) except Exception: diff --git a/hermes_cli/install_identity.py b/hermes_cli/install_identity.py index 2f25436359..46e9c8b191 100644 --- a/hermes_cli/install_identity.py +++ b/hermes_cli/install_identity.py @@ -69,7 +69,8 @@ def read_or_create_install_id(root: Path | None = None) -> Optional[str]: if not mint: return existing try: - root.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + mkdir_under_hermes_home(root) # Windows byte-range locks can report a same-process conflict instead of waiting for another # thread: serialize threads here, then keep the file lock as the cross-process publication fence. with _INSTALL_ID_PUBLICATION_LOCK, _install_id_file_lock(root): diff --git a/hermes_cli/local_runtime/supervisor.py b/hermes_cli/local_runtime/supervisor.py index 0688d3debc..6870efaca9 100644 --- a/hermes_cli/local_runtime/supervisor.py +++ b/hermes_cli/local_runtime/supervisor.py @@ -224,7 +224,8 @@ class LlamaServerSupervisor: "executable": proc.exe(), "owner_pid": os.getpid(), "owner_create_time": psutil.Process().create_time()} path = state_path() - path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + mkdir_under_hermes_home(path.parent) atomic_json_write(path, self._state, mode=0o600) def _wait_health(self, timeout_s: int) -> None: diff --git a/hermes_cli/personality.py b/hermes_cli/personality.py index e2dc163125..22cf01f39b 100644 --- a/hermes_cli/personality.py +++ b/hermes_cli/personality.py @@ -134,7 +134,8 @@ def persist_personality(value: Any) -> bool: from utils import atomic_roundtrip_yaml_update config_path = get_hermes_home() / "config.yaml" - config_path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + mkdir_under_hermes_home(config_path.parent) atomic_roundtrip_yaml_update(config_path, "display.personality", name) try: os.chmod(config_path, 0o600) diff --git a/hermes_cli/plugin_catalog.py b/hermes_cli/plugin_catalog.py index dad4d8ac91..abc8a5ab52 100644 --- a/hermes_cli/plugin_catalog.py +++ b/hermes_cli/plugin_catalog.py @@ -238,6 +238,8 @@ def fetch_live_catalog(*, force: bool = False) -> Optional[Dict[str, Any]]: logger.debug("Plugin catalog: unreadable live cache %s: %s", cache, exc) try: import httpx + from hermes_constants import mkdir_under_hermes_home + resp = httpx.get(LIVE_CATALOG_URL, timeout=_REQUEST_TIMEOUT, follow_redirects=True) resp.raise_for_status() if len(resp.content) > _MAX_LIVE_BYTES: @@ -245,7 +247,7 @@ def fetch_live_catalog(*, force: bool = False) -> Optional[Dict[str, Any]]: data = resp.json() if not isinstance(data, dict) or not isinstance(data.get("entries"), list): raise ValueError("unexpected live catalog payload") - cache.parent.mkdir(parents=True, exist_ok=True) + mkdir_under_hermes_home(cache.parent) cache.write_text(json.dumps(data), encoding="utf-8") return data except Exception as exc: diff --git a/hermes_cli/process_identity.py b/hermes_cli/process_identity.py index d2ad83aad6..b71a066215 100644 --- a/hermes_cli/process_identity.py +++ b/hermes_cli/process_identity.py @@ -266,7 +266,8 @@ def _append_entry(entry: LedgerEntry) -> bool: ] pruned.append(asdict(entry)) try: - path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + mkdir_under_hermes_home(path.parent) # argv may carry surrogate-escaped bytes (non-UTF-8 paths); ensure_ascii keeps the # utf-8 text handle from raising UnicodeEncodeError (a ValueError, not an OSError). atomic_json_write(path, pruned, mode=0o600, ensure_ascii=True) diff --git a/hermes_cli/terminal_breadcrumbs.py b/hermes_cli/terminal_breadcrumbs.py index 786f5949f3..9dc8ca4ce5 100644 --- a/hermes_cli/terminal_breadcrumbs.py +++ b/hermes_cli/terminal_breadcrumbs.py @@ -84,7 +84,8 @@ def write_breadcrumb(session_id: str, cwd: Optional[str] = None) -> None: if not terminal_id: return directory = _breadcrumbs_dir() - directory.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + mkdir_under_hermes_home(directory) now = time.time() payload = {"session_id": session_id, "cwd": cwd or os.getcwd(), "ts": now} atomic_json_write(directory / terminal_id, payload, indent=None) diff --git a/hermes_cli/web_routers/memory_providers.py b/hermes_cli/web_routers/memory_providers.py index 777326cae8..1364d31a58 100644 --- a/hermes_cli/web_routers/memory_providers.py +++ b/hermes_cli/web_routers/memory_providers.py @@ -164,7 +164,8 @@ def _apply_field_values(provider: ProviderConfigSchema, values: Dict[str, str], def _write_json_0600(path: Path, data: Dict[str, Any]) -> None: from utils import atomic_json_write - path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + mkdir_under_hermes_home(path.parent) atomic_json_write(path, data, mode=0o600) diff --git a/plugins/memory/honcho/cli.py b/plugins/memory/honcho/cli.py index 537b60e15d..8efacbf50c 100644 --- a/plugins/memory/honcho/cli.py +++ b/plugins/memory/honcho/cli.py @@ -149,7 +149,8 @@ def _write_config(cfg: dict, path: Path | None = None) -> None: out = _apply_edits(cfg.snapshot, cfg, disk) elif path.exists(): out = _apply_edits(cfg.snapshot, cfg, _overlay_local(cfg.snapshot, disk)) - path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + mkdir_under_hermes_home(path.parent) atomic_json_write(path, out, mode=0o600) if isinstance(cfg, _ReadConfig): # a later write on the same object applies only edits made after this one cfg.snapshot, cfg.path = copy.deepcopy(dict(cfg)), path diff --git a/plugins/memory/openviking/__init__.py b/plugins/memory/openviking/__init__.py index 25353cd2af..ad8a962561 100644 --- a/plugins/memory/openviking/__init__.py +++ b/plugins/memory/openviking/__init__.py @@ -2216,7 +2216,8 @@ class OpenVikingMemoryProvider(MemoryProvider): logger.debug("Could not safely mark OpenViking session %s pending without a run lock", sid) return try: - path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + mkdir_under_hermes_home(path.parent) atomic_json_write(path, {"session_id": sid, "owner_run_id": self._run_id}, mode=0o600) self._pending_marked_sids.add(sid) except Exception as e: diff --git a/plugins/memory/openviking/_setup.py b/plugins/memory/openviking/_setup.py index 488889337a..130220f516 100644 --- a/plugins/memory/openviking/_setup.py +++ b/plugins/memory/openviking/_setup.py @@ -321,7 +321,8 @@ def _mirror_manual_config_to_openviking_store(*, prompt, select, cancelled, valu return _SETUP_CANCELLED if replace is False: continue - path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + mkdir_under_hermes_home(path.parent) # atomic_json_write creates the temp file 0600 and os.replace()s it: no # half-written config on crash, no chmod-after-write window for the keys. ov.atomic_json_write(path, ov._ovcli_data_from_connection_values(values), mode=0o600) diff --git a/plugins/platforms/feishu/adapter.py b/plugins/platforms/feishu/adapter.py index e1106b2cbc..3d14cd64a8 100644 --- a/plugins/platforms/feishu/adapter.py +++ b/plugins/platforms/feishu/adapter.py @@ -3449,7 +3449,8 @@ class FeishuAdapter(BasePlatformAdapter): def _persist_seen_message_ids(self) -> None: try: - self._dedup_state_path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + mkdir_under_hermes_home(self._dedup_state_path.parent) with self._dedup_lock: recent = self._seen_message_order[-self._dedup_cache_size:] # Save as {msg_id: timestamp} so TTL filtering works across restarts. diff --git a/plugins/platforms/google_chat/oauth.py b/plugins/platforms/google_chat/oauth.py index 623f3e7839..b0dfe0fa35 100644 --- a/plugins/platforms/google_chat/oauth.py +++ b/plugins/platforms/google_chat/oauth.py @@ -195,7 +195,8 @@ def _chmod_quiet(path: Path, mode: int) -> None: def _write_private_json(path: Path, data: Any) -> None: """Atomically write JSON with 0o600 permissions (0o700 parent) where supported.""" - path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + mkdir_under_hermes_home(path.parent) _chmod_quiet(path.parent, 0o700) # mkstemp's 0o600 temp + atomic rename never exposes the token at process umask. atomic_write_text(path, json.dumps(data, indent=2, ensure_ascii=False), create_mode=0o600) diff --git a/tools/bot_relay.py b/tools/bot_relay.py index 8544e06400..e65cdcdb5b 100644 --- a/tools/bot_relay.py +++ b/tools/bot_relay.py @@ -86,7 +86,8 @@ def relay_root(root: Path | str) -> Path: def _ensure_dirs(root: Path | str) -> Path: base = relay_root(root) for sub in (OUTBOX_DIR, CLAIMED_DIR, REPLIES_DIR): - (base / sub).mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + mkdir_under_hermes_home(base / sub) return base diff --git a/tools/process_registry_results.py b/tools/process_registry_results.py index b2cd175978..eddc9eba28 100644 --- a/tools/process_registry_results.py +++ b/tools/process_registry_results.py @@ -57,6 +57,8 @@ def save_completed_result(session) -> None: record["command"] = redact_sensitive_text(record["command"], code_file=True, force=True) directory = get_hermes_home() / "logs" / "process-results" try: + from hermes_constants import assert_named_profile_home_live + assert_named_profile_home_live(directory) directory.mkdir(mode=0o700, parents=True, exist_ok=True) atomic_json_write(directory / f"{session.id}.json", record, mode=0o600) _result_paths() diff --git a/tools/registry.py b/tools/registry.py index 28564b024e..aca8baa4c4 100644 --- a/tools/registry.py +++ b/tools/registry.py @@ -170,7 +170,8 @@ def _save_discovery_cache(cache: Dict[str, list]) -> None: return try: from utils import atomic_json_write # stdlib+yaml only; no cycle - path.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + mkdir_under_hermes_home(path.parent) atomic_json_write(path, cache, indent=0) except Exception as e: logger.debug("Could not write tool discovery cache %s: %s", path, e) diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index db87bee81a..8595840e09 100644 --- a/tools/skill_manager_tool.py +++ b/tools/skill_manager_tool.py @@ -364,7 +364,8 @@ def _guarded_write(name: str, skill_dir: Path, target: Path, action: str, label: if read_guard := _background_review_read_before_write_guard(name, target, action, label): return read_guard original = target.read_text(encoding="utf-8") - target.parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + mkdir_under_hermes_home(target.parent) atomic_write_text(target, content, preserve_mode=True, create_mode=0o644) scan_error = _security_scan_skill(skill_dir) if not scan_error: @@ -421,7 +422,8 @@ def _create_skill(name: str, content: str, category: str = None) -> Dict[str, An if existing := _find_skill(name): return _err(f"A skill named '{name}' already exists at {existing['path']}.") skill_dir = _resolve_skill_dir(name, category) - skill_dir.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + mkdir_under_hermes_home(skill_dir) skill_md = skill_dir / "SKILL.md" atomic_write_text(skill_md, content, preserve_mode=True, create_mode=0o644) if scan_error := _security_scan_skill(skill_dir): diff --git a/tools/skills_sync.py b/tools/skills_sync.py index ca774820be..812200e255 100644 --- a/tools/skills_sync.py +++ b/tools/skills_sync.py @@ -126,7 +126,8 @@ def _read_suppressed_names() -> set: def _write_manifest(entries: Dict[str, str]): """Atomic v2 write, preserving an existing file's mode/owner (not mkstemp's 0600).""" - _manifest_file().parent.mkdir(parents=True, exist_ok=True) + from hermes_constants import mkdir_under_hermes_home + mkdir_under_hermes_home(_manifest_file().parent) try: data = "".join(f"{n}:{h}\n" for n, h in sorted(entries.items())) atomic_write_text(_manifest_file(), data, tmp_prefix=".bundled_manifest_", preserve_mode=True) From 7dde7a2424a2d6e123f9ed02e58ca26ce427a4d5 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Tue, 15 Sep 2026 23:07:49 -0700 Subject: [PATCH 08/18] fix(gateway): migrate --multiplex resumes from live state, compensates the whole destructive phase, and refuses an unknown default principal Three P1 findings from the review of #111062 (fixes #110850 remainder): - interrupted-detection keyed off `plan.default.has_gateway`, which is true for an installed-but-dead unit; `systemd_install` writes the unit before the start that can still be killed, so an apply interrupted at default/start (or mid-restart of a stopped default) was reported as "already multiplexed" and never recovered. `interrupted` is now derived from the manifest vs the LIVE default's recorded served_profiles: flag on + manifest + not every migrated profile served = resume. - the compensation `try` began after `_remove_secondary_gateways` and the flag write, so a later secondary's stop, a unit's daemon-reload or the config write failing left the first secondary removed with no rollback. The flag write, removals and default bring-up now all sit inside one boundary that rolls back through the manifest; `_preflight_apply` refuses failures knowable from the plan (root system unit without a recorded User=, unresolvable recorded user, an unwritable config.yaml) before any working gateway is stopped. - `_guard_unix_user` blocked an unknown SECONDARY principal but accepted `default_uid is None`; a default system unit whose User= this host cannot resolve is the same boundary from the other side and now blocks the update hook (same known uid still folds, different known uid still refuses). --- hermes_cli/gateway_migrate.py | 87 +++++++++++--- hermes_cli/gateway_migrate_guards.py | 8 +- .../test_gateway_migrate_multiplex.py | 113 ++++++++++++++++++ .../docs/user-guide/multi-profile-gateways.md | 20 ++-- 4 files changed, 202 insertions(+), 26 deletions(-) diff --git a/hermes_cli/gateway_migrate.py b/hermes_cli/gateway_migrate.py index bd1c7ae860..6dca0ba71b 100644 --- a/hermes_cli/gateway_migrate.py +++ b/hermes_cli/gateway_migrate.py @@ -88,8 +88,10 @@ class MigrationPlan: profiles: list[ProfileGateway] multiplex_flag_on: bool live_served: Optional[list[str]] # served_profiles the live default gateway recorded, if any - # A manifest with the flag on and no live default gateway: an earlier apply died between flipping - # the flag and bringing the multiplexer up (#110850). Not "already multiplexed" — resumable. + # A manifest with the flag on and no LIVE default gateway: an earlier apply died between flipping + # the flag and the multiplexer confirming it is up (#110850). An installed unit is not proof of + # anything — `systemd_install` writes the unit before the start that can still fail or be killed. + # Not "already multiplexed" — resumable. interrupted: bool = False blockers: list[str] = field(default_factory=list) notices: list[str] = field(default_factory=list) @@ -449,7 +451,7 @@ def build_migration_plan() -> MigrationPlan: multiplex_flag_on=_read_multiplex_flag(default_home), live_served=recorded_served_profiles(default_home), ) - plan.interrupted = plan.multiplex_flag_on and not plan.default.has_gateway and _read_manifest(default_home) is not None + plan.interrupted = plan.multiplex_flag_on and _manifest_not_yet_served(_read_manifest(default_home), plan.live_served) if len(plan.profiles) < 2: plan.notices.append("Only one profile exists: nothing to multiplex.") return plan @@ -487,7 +489,7 @@ def format_plan(plan: MigrationPlan, *, dry_run: bool) -> list[str]: return lines if plan.interrupted: lines.append(f" ↻ An earlier migration was interrupted before the default gateway came up " - f"(flag on, no gateway; manifest {plan.default_home / MANIFEST_NAME}); this run resumes it.") + f"(flag on, no live multiplexer; manifest {plan.default_home / MANIFEST_NAME}); this run resumes it.") steps = [] for p in plan.standalone_secondaries: what = " + ".join(x for x in (f"stop pid {p.pid}" if p.pid else "", f"uninstall {p.service_label()}" if p.services else "") if x) @@ -615,6 +617,19 @@ def _read_manifest(default_home: Path) -> Optional[dict]: return data if isinstance(data, dict) else None +def _manifest_not_yet_served(manifest: Optional[dict], live_served: Optional[list[str]]) -> bool: + """The postcondition ``apply_migration`` waits for, re-derived from live state: a LIVE default that + recorded serving every profile the manifest migrated. Anything less — no live gateway, an + installed-but-dead unit (``systemd_install`` writes the unit before the start that can still fail), + a standalone default never restarted — is a half-applied migration, not "already multiplexed". + Profiles created after the migration are not in the manifest, so they cannot flag it as interrupted.""" + if manifest is None: + return False + recs = [r for r in (manifest.get("default"), *(_manifest_secondaries(manifest) or [])) if isinstance(r, dict)] + migrated = {str(r.get("profile") or "default") for r in recs} | {"default"} + return not migrated <= set(live_served or []) + + def _write_manifest(default_home: Path, data: dict) -> None: from utils import atomic_json_write @@ -691,14 +706,48 @@ def _remove_secondary_gateways(plan: MigrationPlan) -> None: print(f" ✓ {p.name}: stopped standalone gateway (pid {p.pid})") +def _preflight_apply(plan: MigrationPlan, target: Optional[tuple[str, bool]], run_as_user: Optional[str]) -> Optional[str]: + """A failure of the destructive phase that is knowable from the plan alone, refused BEFORE any + working per-profile gateway is stopped: rollback is the fallback for surprises, not the plan. + Mirrors the checks ``systemd_install``/``_service_call`` make on a system unit (root, resolvable + ``User=``) and the config write's read-guard.""" + from hermes_cli import gateway as gw + from hermes_cli.config import require_readable_config_before_write + try: + require_readable_config_before_write(plan.default_home / "config.yaml") + except Exception as exc: + return f"default: config.yaml cannot be updated ({exc})" + touches_system_unit = target == ("systemd", True) or any(p.has_system_unit for p in plan.standalone_secondaries) + if touches_system_unit: + try: + gw._require_root_for_system_service("migration") + except Exception as exc: + return str(exc) + if plan.default.service is None and target == ("systemd", True): + if run_as_user is None: + try: + gw._system_service_identity() # the #110850 refusal (implicit root), before anything is removed + except ValueError as exc: + return f"default: {exc}" + else: + import pwd + try: + pwd.getpwnam(run_as_user) + except KeyError: + return f"default: the recorded service user '{run_as_user}' does not exist on this host" + return None + + def apply_migration(plan: MigrationPlan, *, served_wait: float = _SERVED_WAIT_SECONDS) -> bool: - """Stop/uninstall every secondary gateway, flip the flag, bring up the multiplexer, verify. + """Flip the flag, stop/uninstall every secondary gateway, bring up the multiplexer, verify. Returns True when the multiplexer verifiably serves every profile. - Bringing the default up is the one step that can fail after the destructive ones (a system unit - that needs ``--run-as-user``, an unreachable user bus). It runs inside a rollback: on failure the - manifest written before the first destructive step restores the flag and every recorded per-profile - gateway (#110850), so the fleet never ends with the flag on and no gateway at all.""" + Every step after the manifest write is fallible (a config write, a secondary's stop or its + unit's daemon-reload, a system unit that needs ``--run-as-user``, an unreachable user bus) and + runs inside ONE compensating boundary: on failure the manifest written before the first + destructive step restores the flag and every recorded per-profile gateway (#110850), so the + fleet never ends half-migrated. The flag goes on first so an apply killed anywhere after it is + resumable from the manifest (flag on + manifest + no live multiplexer = interrupted).""" if plan.blocked: _print(["✗ Migration refused:", *[f" • {b}" for b in plan.blockers]]) return False @@ -707,18 +756,22 @@ def apply_migration(plan: MigrationPlan, *, served_wait: float = _SERVED_WAIT_SE return True target, run_as_user = plan.target_service_kind(), plan.target_run_as_user() if plan.interrupted: - # An earlier apply removed the secondaries and flipped the flag but never brought the default - # up; the manifest is the only record of the units that existed. Finish from it, don't rewrite it. + # An earlier apply flipped the flag (and removed some or all secondaries) but the default never + # came up; the manifest is the only record of the units that existed. Finish from it, don't rewrite it. manifest = _read_manifest(plan.default_home) or {} target, run_as_user = _target_from_manifest(manifest) print(f" ↻ resuming an interrupted migration recorded in {_manifest_path(plan.default_home)}") elif _read_manifest(plan.default_home) is not None: - # Flag off + manifest present = a rollback that did not finish. Overwriting the manifest would - # discard the only record of the units that rollback still has to restore. + # Flag off + manifest present = a rollback (or an apply killed before its flag write) that did + # not finish. Overwriting the manifest would discard the only record of the units to restore. _print([f"✗ A previous migration's manifest is still at {_manifest_path(plan.default_home)} (its rollback did not finish).", " Finish it with: hermes gateway migrate --standalone (or delete the manifest to start over)"]) return False else: + blocker = _preflight_apply(plan, target, run_as_user) + if blocker is not None: + _print(["✗ Migration refused before changing anything:", f" • {blocker}"]) + return False manifest = { "version": 1, "migrated_at": time.strftime("%Y-%m-%dT%H:%M:%S%z"), "flag_was": plan.multiplex_flag_on, @@ -728,13 +781,13 @@ def apply_migration(plan: MigrationPlan, *, served_wait: float = _SERVED_WAIT_SE # Recovery metadata must exist before the first destructive operation; the manifest never # changes afterwards, so this is the only write it needs. _write_manifest(plan.default_home, manifest) - _remove_secondary_gateways(plan) - _write_multiplex_flag(plan.default_home, True) - print(f" ✓ default: gateway.multiplex_profiles: true ({plan.default_home / 'config.yaml'})") try: + _write_multiplex_flag(plan.default_home, True) + print(f" ✓ default: gateway.multiplex_profiles: true ({plan.default_home / 'config.yaml'})") + _remove_secondary_gateways(plan) # on resume: whatever an apply killed mid-removal left installed print(f" ✓ {_restart_default(plan.default, target, plan.default_home, run_as_user=run_as_user)}") except Exception as exc: - _print([f" ✗ default: could not bring up the multiplexed gateway ({exc})", + _print([f" ✗ migration failed ({exc})", " ↩ Rolling back to per-profile gateways so no profile is left without one..."]) rolled_back = rollback_migration(plan.default_home) if not rolled_back: diff --git a/hermes_cli/gateway_migrate_guards.py b/hermes_cli/gateway_migrate_guards.py index a7a04b3d99..cadbb14ac7 100644 --- a/hermes_cli/gateway_migrate_guards.py +++ b/hermes_cli/gateway_migrate_guards.py @@ -105,6 +105,11 @@ def _guard_service_domain(plan: MigrationPlan, profile: ProfileGateway) -> Optio def _guard_unix_user(plan: MigrationPlan, profile: ProfileGateway) -> Optional[str]: default_uid = plan.default.uid + if default_uid is None and plan.default.has_system_unit: + # Consolidating INTO a principal this host cannot identify is the same unknown boundary + # from the other side: the default's system unit names an account NSS does not resolve. + return ("The default gateway runs a system unit whose User= cannot be resolved on this host: " + "an unknown service principal is not folded into automatically.") if profile.uid is None and profile.has_system_unit: # Unknown principal is not "same user": the unit names an account this host cannot resolve. return (f"Profile '{profile.name}' runs a system unit whose User= cannot be resolved on this host: " @@ -134,12 +139,13 @@ _AUTO_MIGRATION_GUARDS: tuple[Callable[[MigrationPlan, ProfileGateway], Optional def auto_migration_blockers(plan: MigrationPlan) -> list[str]: """Every boundary a standalone secondary sits behind; empty when the fleet is one user, one service domain, one profiles/ tree — the only shape ``hermes update`` may fold on its own.""" - return [ + findings = [ finding for profile in plan.standalone_secondaries for guard in _AUTO_MIGRATION_GUARDS if (finding := guard(plan, profile)) is not None ] + return list(dict.fromkeys(findings)) # a default-side finding repeats per secondary # --------------------------------------------------------------------------- opt-out diff --git a/tests/hermes_cli/test_gateway_migrate_multiplex.py b/tests/hermes_cli/test_gateway_migrate_multiplex.py index d1f1fe008f..8952fd28ba 100644 --- a/tests/hermes_cli/test_gateway_migrate_multiplex.py +++ b/tests/hermes_cli/test_gateway_migrate_multiplex.py @@ -78,6 +78,8 @@ def fleet(tmp_path, monkeypatch): monkeypatch.setattr(gm, "_service_op", _service_op) monkeypatch.setattr(gm, "_stop_gateway_process", lambda home: state.pids.pop(_name(home), None)) monkeypatch.setattr(gm, "_host_supports_migration", lambda: None) + # Part of the faked service layer: the real check asks systemd's questions (root, NSS user). + monkeypatch.setattr(gm, "_preflight_apply", lambda plan, target, run_as_user: None) state.root = root return state @@ -92,6 +94,9 @@ def _units(recorded) -> list: return list(recorded) if isinstance(recorded, list) else [recorded] +_real_preflight = gm._preflight_apply + + def _config_flag(root: Path): import yaml raw = yaml.safe_load((root / "config.yaml").read_text(encoding="utf-8")) or {} @@ -564,3 +569,111 @@ def test_opt_out_reads_effective_config_managed_false_wins_and_string_false_is_f assert auto_migration_opted_out(fleet.root) is True gm.maybe_auto_migrate_after_update() assert fleet.ops == [] and _config_flag(fleet.root) is None + + +@pytest.mark.parametrize("default_unit_preinstalled", [False, True]) +def test_interruption_after_the_default_unit_exists_is_still_interrupted_not_already_multiplexed( + fleet, monkeypatch, capsys, default_unit_preinstalled, +): + """``systemd_install`` writes the unit before the start that can be killed; an existing stopped + default unit can be interrupted mid-restart. Either way the flag is on and a default unit exists + but nothing serves anyone: that is an interrupted migration to resume, not a completed one. Only + a LIVE multiplexer that recorded ``served_profiles`` counts as already multiplexed.""" + if default_unit_preinstalled: + fleet.services["default"] = ("systemd", False) + real_op = gm._service_op + + def _killed_at_start(kind, system, verb, home, *, run_as_user=None): + if verb in ("start", "restart") and _name(home) == "default": + raise KeyboardInterrupt() + real_op(kind, system, verb, home, run_as_user=run_as_user) + + with pytest.MonkeyPatch.context() as dying: + dying.setattr(gm, "_service_op", _killed_at_start) + with pytest.raises(KeyboardInterrupt): + gm.apply_migration(gm.build_migration_plan(), served_wait=0.1) + assert _config_flag(fleet.root) is True and fleet.services == {"default": ("systemd", False)} + assert (fleet.root / gm.MANIFEST_NAME).exists() + + plan = gm.build_migration_plan() + assert plan.interrupted and not plan.already_multiplexed + fleet.ops.clear() + assert gm.apply_migration(plan, served_wait=5.0) is True + assert fleet.ops[-1] == ("default", "restart") and "serves 3 profiles" in capsys.readouterr().out + # Postcondition met: the next plan sees the live multiplexer and stops. + assert gm.build_migration_plan().already_multiplexed + + +@pytest.mark.parametrize("failing_op", [("ops", "stop"), ("coder", "uninstall"), ("default", "flag")]) +def test_failure_anywhere_in_the_destructive_phase_restores_the_removed_secondaries(fleet, monkeypatch, capsys, failing_op): + """The compensation boundary covers the whole destructive phase, not only the default bring-up: + a later secondary's stop, a unit unlink/daemon-reload, or the flag write failing after an earlier + secondary was removed must put that secondary back (the manifest alone is not a restored fleet).""" + name, verb = failing_op + real_op = gm._service_op + + def _failing(kind, system, verb_, home, *, run_as_user=None): + if (_name(home), verb_) == (name, verb): + raise RuntimeError(f"{name} {verb} failed") + real_op(kind, system, verb_, home, run_as_user=run_as_user) + + monkeypatch.setattr(gm, "_service_op", _failing) + if verb == "flag": + real_flag = gm._write_multiplex_flag + monkeypatch.setattr(gm, "_write_multiplex_flag", + lambda home, value: (_ for _ in ()).throw(OSError("read-only config")) if value else real_flag(home, value)) + assert gm.apply_migration(gm.build_migration_plan(), served_wait=0.1) is False + out = capsys.readouterr().out + assert "Rolling back" in out and "Rolled back" in out + assert _config_flag(fleet.root) is not True + assert fleet.services == {"coder": ("systemd", False), "ops": ("systemd", False)} + assert not (fleet.root / gm.MANIFEST_NAME).exists() + assert gm.build_migration_plan().eligible_for_migration() + + +def test_known_bringup_refusal_is_rejected_before_any_secondary_is_touched(fleet, monkeypatch, capsys): + """A system-unit fleet with no recorded User= run by root is the #110850 refusal: known from the plan, + so it is refused before a working gateway is stopped rather than discovered and rolled back.""" + from hermes_cli import gateway as gw + fleet.services.update({"coder": ("systemd", True), "ops": ("systemd", True)}) + monkeypatch.setattr(gm, "_systemd_service_user", lambda home, services: None) + monkeypatch.setattr(gm, "_preflight_apply", _real_preflight) + monkeypatch.setattr(gw, "_require_root_for_system_service", lambda action: None) # we are "root" + for var in ("SUDO_USER", "USER", "LOGNAME"): + monkeypatch.setenv(var, "root") + assert gm.apply_migration(gm.build_migration_plan(), served_wait=0.1) is False + out = capsys.readouterr().out + assert "before changing anything" in out and "--run-as-user root" in out + assert fleet.ops == [] and _config_flag(fleet.root) is None and not (fleet.root / gm.MANIFEST_NAME).exists() + + +def test_unknown_default_system_principal_blocks_the_update_hook(fleet, tmp_path, monkeypatch, capsys): + """Mirror of the unknown-secondary case: the default's system unit names a User= this host cannot + resolve while both secondaries are known root system units. Folding INTO an unidentifiable + principal is the same boundary; known-same uid still folds, known-different still refuses.""" + from hermes_cli import gateway as gw + from hermes_cli.gateway_migrate_guards import auto_migration_blockers, gateway_identity + unit_dir = tmp_path / "system"; unit_dir.mkdir() + monkeypatch.setattr(gw, "_SYSTEM_UNIT_DIR", unit_dir) + with gm._home_env(fleet.root): + gw.get_systemd_unit_path(system=True).write_text("[Service]\nUser=no-such-pr111062-user\n", encoding="utf-8") + for name in ("coder", "ops"): + with gm._home_env(fleet.root / "profiles" / name): + gw.get_systemd_unit_path(system=True).write_text("[Service]\nUser=root\n", encoding="utf-8") + fleet.services.update({"default": ("systemd", True), "coder": ("systemd", True), "ops": ("systemd", True)}) + fleet.pids.clear() # stopped units everywhere: identity comes from User=, resolved for real + plan = gm.build_migration_plan() + assert plan.default.uid is None and {p.uid for p in plan.standalone_secondaries} == {0} + assert gateway_identity(fleet.root, None, [("systemd", True)])[0] is None + blockers = auto_migration_blockers(plan) + assert len(blockers) == 1 and "default gateway" in blockers[0] and "cannot be resolved" in blockers[0] + gm.maybe_auto_migrate_after_update() + out = capsys.readouterr().out + assert "cannot be resolved" in out and gm.MIGRATE_COMMAND in out + assert fleet.ops == [] and _config_flag(fleet.root) is None + + # Controls: same known uid folds; a different known uid refuses. + monkeypatch.setattr(gm, "_gateway_identity", lambda home, pid, services: (0, home)) + assert auto_migration_blockers(gm.build_migration_plan()) == [] + monkeypatch.setattr(gm, "_gateway_identity", lambda home, pid, services: (0 if _name(home) == "default" else 1000, home)) + assert any("UNIX privilege boundary" in b for b in auto_migration_blockers(gm.build_migration_plan())) diff --git a/website/docs/user-guide/multi-profile-gateways.md b/website/docs/user-guide/multi-profile-gateways.md index ecb4333e3d..2398580b58 100644 --- a/website/docs/user-guide/multi-profile-gateways.md +++ b/website/docs/user-guide/multi-profile-gateways.md @@ -854,7 +854,7 @@ A standalone secondary behind any of these boundaries stops the automatic path: |---|---| | different service manager or scope | default on user systemd, a secondary on **system** systemd (or launchd), or the default detached with a service-managed secondary | | more than one installed unit on a profile | a user **and** a system unit for the same profile (the explicit command removes both) | -| different UNIX user | a system unit with its own `User=`, or a live gateway owned by another uid; a system unit whose `User=` this host cannot resolve counts as unknown, never as "same user" | +| different UNIX user | a system unit with its own `User=`, or a live gateway owned by another uid; a system unit whose `User=` this host cannot resolve — on the secondary **or** on the default — counts as unknown, never as "same user" | | `HERMES_HOME` outside `/profiles/` | a unit pinning `HERMES_HOME=/opt/hermes/profiles/emma` | In that case `hermes update` prints the boundary it found plus @@ -957,13 +957,17 @@ previous value, restarts the default gateway, and reinstalls/starts every recorded per-profile service (a system unit comes back with the `User=` it had). The manifest is removed once everything is back. -The forward migration is transactional in the same way: if bringing the default -gateway up fails after the per-profile gateways were removed (for example a -system unit that has to run as root), `--multiplex` rolls back through the -manifest on the spot so no profile is left without a gateway. Should the -process die between flipping the flag and starting the default, the next -`hermes gateway migrate --multiplex` sees the manifest with no live gateway and -resumes from it instead of reporting "already multiplexed". +The forward migration is transactional in the same way. Failures it can see +coming from the plan (a system unit that would have to run as root without a +recorded `User=`, a config file it cannot rewrite) are refused before any +per-profile gateway is stopped. Anything that fails after the manifest is +written — the flag write, a later secondary's stop or unit removal, the +default's install or start — rolls back through the manifest on the spot, so no +profile is left without a gateway. Should the process die anywhere in that +window, the next `hermes gateway migrate --multiplex` sees the flag on, the +manifest, and no live multiplexer serving the migrated profiles (an installed +but stopped default unit does not count) and resumes from the manifest instead +of reporting "already multiplexed". If no manifest exists (you enabled multiplexing by hand), leave multiplex mode with `hermes config set gateway.multiplex_profiles false && hermes gateway restart` and reinstall the per-profile services you want. From 3fe8e5e443d1c30843296679e86d90f335aa1269 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Wed, 16 Sep 2026 00:03:34 -0700 Subject: [PATCH 09/18] fix(multiplex): routed children never inherit launch-only credentials, with or without the multiplex flag Two authority gaps in served_profile_child_env (#111617 review, andrexibiza P1 #1/#2, kvnloo finding 1): - The base was hermes_subprocess_env(inherit_credentials=True) = the launch environ's provider credentials; strip_launch_profile_env only knows names with .env/source provenance, so a key systemd/Compose/the shell injected into the launch process survived into profile B's child whenever B did not define the same name. Now a ROUTED target scrubs every Tier-1/Tier-2 credential from the base regardless of provenance before B's own scope is overlaid (the child boundary gets get_secret's contract: a scoped miss is no credential, never ambient fallback). The launch profile's own child keeps its env. bot_relay's base=os.environ goes through the same scrub. - strip_launch_profile_env / the scrub keyed on is_multiplex_active(); the Desktop and dashboard backends serve ?profile=B by installing the HERMES_HOME override without that flag, so B's slash worker / helper children kept A's .env and settings. The authority test is now "is the target a routed home" (target != process home). - _build_browser_env resolved the passthrough keys via get_secret, which falls through to os.environ on a scoped miss while multiplexing is inactive: a routed B with no Firecrawl key got A's. Under serves_routed_profile() the bound scope is the only source. - served_profile_child_env(inherit_credentials=True) with no target and no scope bound under multiplex minted with the launch credentials (key_cmd TTL refresh on a worker thread); it now raises UnscopedSecretError like get_secret. tests/tui_gateway/test_served_profile_child_env_authority.py: ambient-only A key + B missing it (mux on), flag-off routed B (helper child + browser), real child observation. 3/3 red on base. --- ...test_served_profile_child_env_authority.py | 98 +++++++++++++++++++ tools/browser_tool.py | 12 ++- tools/environments/local.py | 74 +++++++++----- 3 files changed, 157 insertions(+), 27 deletions(-) create mode 100644 tests/tui_gateway/test_served_profile_child_env_authority.py diff --git a/tests/tui_gateway/test_served_profile_child_env_authority.py b/tests/tui_gateway/test_served_profile_child_env_authority.py new file mode 100644 index 0000000000..e1303003cf --- /dev/null +++ b/tests/tui_gateway/test_served_profile_child_env_authority.py @@ -0,0 +1,98 @@ +"""A routed profile's child never receives a launch-profile credential — whatever its provenance. + +Two authority edges of ``served_profile_child_env`` (review of #111617): + +* a credential injected into the LAUNCH process by systemd / Compose / the shell is in no ``.env`` + and no source snapshot, so a name-based strip cannot see it; the child for routed profile B must + still not carry it when B does not define the same name (``get_secret``'s multiplex contract: + a scoped miss is *no credential*, never ambient fallback); +* the Desktop/dashboard backend serves ``?profile=B`` by installing a HERMES_HOME override WITHOUT + the gateway-wide multiplex flag — the strip must key on "this task serves a routed home", not on + that flag; the browser passthrough must not fall through to the launch key on a B miss either. +""" + +import os +import sys +from pathlib import Path + +import pytest + +from agent.secret_scope import UnscopedSecretError, set_multiplex_active +from hermes_constants import reset_hermes_home_override, set_hermes_home_override +from tools.environments.local import served_profile_child_env + + +@pytest.fixture +def homes(tmp_path, monkeypatch): + """Launch home A (its .env in os.environ) plus an AMBIENT-only OPENAI key A never wrote to a file; + served home B defines neither.""" + a = tmp_path / ".hermes" + b = a / "profiles" / "b" + b.mkdir(parents=True) + (a / ".env").write_text("A_MARKER=a\nHERMES_MODEL=a-model\nFIRECRAWL_API_KEY=a-fc\n", encoding="utf-8") + (b / ".env").write_text("B_MARKER=b\n", encoding="utf-8") + monkeypatch.setenv("HERMES_HOME", str(a)) + for key, val in (("A_MARKER", "a"), ("HERMES_MODEL", "a-model"), ("FIRECRAWL_API_KEY", "a-fc"), + ("OPENAI_API_KEY", "sk-ambient-launch-only")): + monkeypatch.setenv(key, val) + monkeypatch.delenv("B_MARKER", raising=False) + set_multiplex_active(False) + try: + yield a, b + finally: + set_multiplex_active(False) + + +def test_ambient_only_launch_credential_never_reaches_a_routed_child(homes): + """Multiplex on: A's ambient OPENAI_API_KEY (not in A's .env) is absent from B's credential-bearing + child while B's own secrets are present; with no target and no scope bound the builder refuses.""" + a, b = homes + set_multiplex_active(True) + env = served_profile_child_env(target_home=b, inherit_credentials=True) + assert env["HERMES_HOME"] == str(b) and env["B_MARKER"] == "b" + assert "OPENAI_API_KEY" not in env and "A_MARKER" not in env and "FIRECRAWL_API_KEY" not in env + # The raw-environ base used by the relay delivery child is scrubbed the same way. + env = served_profile_child_env(base=os.environ, target_home=b, inherit_credentials=True) + assert "OPENAI_API_KEY" not in env and env["B_MARKER"] == "b" + # The launch profile's own child keeps its env (ambient injection is A's legitimate credential). + assert served_profile_child_env(target_home=a, inherit_credentials=True)["OPENAI_API_KEY"] == "sk-ambient-launch-only" + with pytest.raises(UnscopedSecretError): + served_profile_child_env(inherit_credentials=True) + + +def test_routed_home_with_multiplex_flag_off_gets_no_launch_residue(homes, monkeypatch): + """Desktop/dashboard topology: B served via the HERMES_HOME override only. The slash-worker / + helper child sees B's env, and the browser passthrough resolves B's (absent) key as no key.""" + from tools.browser_tool import _build_browser_env + + a, b = homes + token = set_hermes_home_override(str(b)) + try: + env = served_profile_child_env(inherit_credentials=True) + assert env["HERMES_HOME"] == str(b) and env["B_MARKER"] == "b" + assert "A_MARKER" not in env and "HERMES_MODEL" not in env and "OPENAI_API_KEY" not in env + browser_env = _build_browser_env() + assert "FIRECRAWL_API_KEY" not in browser_env and browser_env["HERMES_HOME"] == str(b) + finally: + reset_hermes_home_override(token) + # Control: the launch profile's own browser keeps its key and its settings. + assert _build_browser_env()["FIRECRAWL_API_KEY"] == "a-fc" + assert served_profile_child_env(inherit_credentials=True)["HERMES_MODEL"] == "a-model" + + +def test_real_child_observes_only_the_routed_profile(homes): + """Observed from inside a real child spawned for B with the flag off.""" + import json + import subprocess + + a, b = homes + token = set_hermes_home_override(str(b)) + try: + env = served_profile_child_env(inherit_credentials=True) + finally: + reset_hermes_home_override(token) + probe = "import json,os;print(json.dumps({k:os.environ.get(k) for k in ('HERMES_HOME','A_MARKER','B_MARKER','OPENAI_API_KEY')}))" + out = subprocess.run([sys.executable, "-c", probe], env=env, capture_output=True, text=True, timeout=60) + seen = json.loads(out.stdout.strip().splitlines()[-1]) + assert seen == {"HERMES_HOME": str(b), "A_MARKER": None, "B_MARKER": "b", "OPENAI_API_KEY": None} + assert Path(seen["HERMES_HOME"]) == b diff --git a/tools/browser_tool.py b/tools/browser_tool.py index d6a7cb4a15..b8576e4f61 100644 --- a/tools/browser_tool.py +++ b/tools/browser_tool.py @@ -40,17 +40,19 @@ def _build_browser_env() -> dict: harnesses stub the ``tools`` package). The passthrough keys are re-added from the active profile's secret scope, never ``os.environ``: under multiplex that holds the LAUNCH profile's Browserbase/Firecrawl keys, and a served profile's browser must run on its own (or none).""" - from agent.secret_scope import UnscopedSecretError, get_secret + from agent.secret_scope import current_secret_scope, get_secret, serves_routed_profile from tools.environments.local import served_profile_child_env from agent.proxy_bypass import add_loopback_no_proxy env = served_profile_child_env(inherit_credentials=False) + # A routed profile (multiplex, or a Desktop/dashboard backend serving ``?profile=B`` with the + # flag off) resolves from its bound scope only — a miss is "no key", never the launch profile's + # ``os.environ`` value that ``get_secret`` falls through to while multiplexing is inactive. + routed = serves_routed_profile() + scope = (current_secret_scope() or {}) if routed else None for key in _BROWSER_PASSTHROUGH_KEYS: - try: - value = get_secret(key) - except UnscopedSecretError: - value = None # multiplex, no scope bound: no key rather than a sibling profile's + value = scope.get(key) if routed else get_secret(key) if value is not None: env[key] = value # The Browser Use harness dials the resolved local CDP URL over ``websockets``; without a diff --git a/tools/environments/local.py b/tools/environments/local.py index d38c636167..6a82637616 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -306,7 +306,13 @@ def hermes_subprocess_env(*, inherit_credentials: bool = False) -> dict[str, str provider/tool blocklist) unless ``inherit_credentials`` — pass that **only** for children that legitimately need LLM credentials (user-blessed claude/codex/gemini CLI, TUI Node host). Terminal/execute_code use ``_sanitize_subprocess_env``.""" - env = os.environ.copy() + env = _scrub_credentials(os.environ.copy(), inherit_credentials=inherit_credentials) + env.setdefault("PYTHONUTF8", "1") # Windows UTF-8 safety for spawned processes + return _finalize_child_env(env) + + +def _scrub_credentials(env: dict, *, inherit_credentials: bool) -> dict: + """Tier 1 (always) and, unless ``inherit_credentials``, Tier 2 provider/tool credentials, in place.""" strip = _ALWAYS_STRIP_KEYS | _plugin_terminal_env_strip_keys() if not inherit_credentials: strip |= _HERMES_PROVIDER_ENV_BLOCKLIST @@ -314,8 +320,7 @@ def hermes_subprocess_env(*, inherit_credentials: bool = False) -> dict[str, str if (key in strip or key.startswith(_HERMES_PROVIDER_ENV_FORCE_PREFIX) or _is_hermes_internal_secret(key)): del env[key] - env.setdefault("PYTHONUTF8", "1") # Windows UTF-8 safety for spawned processes - return _finalize_child_env(env) + return env def build_subprocess_env( @@ -342,44 +347,69 @@ def served_profile_child_env( inherit_credentials: bool = False, ) -> dict[str, str]: """Child env for a process that acts FOR the active (possibly served) profile: ``hermes -p X`` - workers, ``key_cmd`` helpers, browser drivers. The process env is the LAUNCH profile's, so its - ``.env`` residue and bridged ``TERMINAL_*`` are dropped (``strip_launch_profile_env``; no-op - outside multiplex) and the target home is pinned. ``inherit_credentials=True`` is for children - that legitimately run with the profile's credentials (they run the agent or mint its token): the - target profile's own secrets (its ``.env`` + hydrated sources, i.e. what a standalone - ``hermes -p X`` loads itself) are overlaid — never a sibling profile's. ``False`` keeps the - provider scrub; the caller re-adds the few keys the child needs via ``get_secret``. - ``target_home`` defaults to the active override; ``base`` replaces the ``hermes_subprocess_env`` - snapshot.""" - from agent.secret_scope import build_profile_secret_scope, current_secret_scope + workers, ``key_cmd`` helpers, browser drivers. The process env is the LAUNCH profile's. When the + target is a ROUTED home (not the launch profile's — under multiplex or a Desktop/dashboard backend + serving ``?profile=`` with the flag off) the launch ``.env`` residue and bridged ``TERMINAL_*`` are + dropped (``strip_launch_profile_env``) AND every provider/tool credential is scrubbed from the base + regardless of provenance: a key systemd / Compose / the shell injected into the launch process was + never recorded in ``.env`` or a source snapshot, so a name-based strip cannot see it and the target + overlay cannot remove it. ``inherit_credentials=True`` is for children that legitimately run with + the profile's credentials (they run the agent or mint its token): the target profile's own secrets + (its ``.env`` + hydrated sources, what a standalone ``hermes -p X`` loads itself) are overlaid — never + a sibling profile's. Under multiplex with neither a target nor a bound scope the call raises + (``get_secret``'s fail-closed contract): minting with the launch environ would sign in as the wrong + profile. ``False`` keeps the provider scrub; the caller re-adds the few keys the child needs via + ``get_secret``. ``target_home`` defaults to the active override; ``base`` replaces the + ``hermes_subprocess_env`` snapshot.""" + from agent.secret_scope import ( + UnscopedSecretError, build_profile_secret_scope, current_secret_scope, is_multiplex_active) from hermes_constants import get_hermes_home_override env = dict(base) if base is not None else hermes_subprocess_env(inherit_credentials=inherit_credentials) target = str(target_home or get_hermes_home_override() or "") if target: env["HERMES_HOME"] = target - strip_launch_profile_env(env, target) + if _is_routed_home(target): + strip_launch_profile_env(env, target) + _scrub_credentials(env, inherit_credentials=False) if inherit_credentials: - secrets = build_profile_secret_scope(Path(target)) if target else (current_secret_scope() or {}) - env.update((k, v) for k, v in secrets.items() if v is not None) + if target: + secrets = build_profile_secret_scope(Path(target)) + else: + secrets = current_secret_scope() + if secrets is None and is_multiplex_active(): + raise UnscopedSecretError( + "", "served_profile_child_env(inherit_credentials=True) called with no target home and " + "no profile secret scope bound while multiplexing is on; the child would inherit the " + "launch profile's credentials. Bind the profile scope (or pass target_home) at the spawn site.") + env.update((k, v) for k, v in (secrets or {}).items() if v is not None) return env +def _is_routed_home(target_home: "str | Path") -> bool: + """True when ``target_home`` is not the process's own (launch) home.""" + from hermes_constants import get_process_hermes_home + try: + return Path(target_home).resolve() != get_process_hermes_home().resolve() + except OSError: + return True + + def strip_launch_profile_env(env: dict, target_home: "str | Path | None" = None) -> dict: """Drop the LAUNCH profile's residue from a child env built for another served profile. ``os.environ`` holds the default profile's ``.env`` and its bridged ``TERMINAL_*`` settings; the secret scrub removes credentials but not settings (``HERMES_MODEL``, ``TERMINAL_ENV``, ``HERMES_LANGUAGE``...), so a standalone ``hermes -p X`` worker and a served one saw different envs. The child re-loads X's own ``.env`` and bridges X's config itself. ``target_home`` - defaults to the active home override; no-op outside multiplex or when the target IS the - launch profile.""" - from agent.secret_scope import _is_global_env, is_multiplex_active, load_env_file + defaults to the active home override; no-op when there is no target or the target IS the + launch profile. The authority test is "does this task serve a routed home", not "is the + gateway-wide multiplex flag on": the Desktop/dashboard backend serves ``?profile=B`` by + installing a HERMES_HOME override without that flag.""" + from agent.secret_scope import _is_global_env, load_env_file from hermes_constants import get_hermes_home_override, get_process_hermes_home target = target_home or get_hermes_home_override() - if not is_multiplex_active() or not target: + if not target or not _is_routed_home(target): return env launch_home = get_process_hermes_home() - if Path(target).resolve() == launch_home.resolve(): - return env from hermes_cli.config import TERMINAL_CONFIG_ENV_MAP for key in set(load_env_file(launch_home / ".env")) | set(TERMINAL_CONFIG_ENV_MAP.values()): if not _is_global_env(key) or key.startswith("TERMINAL_"): From 86097433893b392e5e963430032950ce0902d749 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Wed, 16 Sep 2026 00:05:37 -0700 Subject: [PATCH 10/18] fix(serve): launch-profile scope decided at entry; send keeps scope authority; per-reset release MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three edges of the fail-closed multi-profile host (#111620 review, andrexibiza P1 + P2, kvnloo finding 1): - `send` under a routed profile's scope `update()`d the installed scope from raw `.env`, reversing build_profile_secret_scope's precedence (user .env, then external secret sources) for the rest of the request; a stale user value beat the secret-manager one. The installed scope is authoritative as-is; only the config.yaml setdefault bridge runs. - The launch profile's body was scoped only when `is_multiplex_active()` was already true at entry, while get_secret consults that global on every read. A launch RPC / dashboard request entering single-profile and resuming after a concurrent first `?profile=B` activation raised UnscopedSecretError mid-request. The launch profile's secret scope (its .env + external sources over the launch env: live while single-profile, the frozen snapshot once multiplexing is active) is now bound for every launch-profile body, so the credential source is fixed at entry. The terminal policy overlay stays multiplex-only (standalone terminal execution keeps its os.environ bridge). _publish_env_value mirrors a same-request .env write into that scope AND os.environ for the launch profile, only into the scope for a routed one (serves_routed_profile). - _release_profile_runtime_scope_tokens reset terminal → secret → home in sequence under one outer suppress; a failing terminal reset left the previous profile's secrets and HERMES_HOME installed for the next body in that context. Each reset is now independent; the first failure is re-raised after every scope is released. tests/tui_gateway/test_multi_profile_hosting_transitions.py: manager-vs-dotenv precedence through _load_hermes_env, TUI-RPC and dashboard barrier tests (launch enters single-profile, B activates on another thread, launch resumes and still resolves its injected credential, never B's), forced terminal-reset failure still releases secret + home. 4/4 red on base. --- hermes_cli/config.py | 15 +- hermes_cli/send_cmd.py | 8 +- hermes_cli/web_server_profiles.py | 17 +-- .../test_multi_profile_hosting_transitions.py | 135 ++++++++++++++++++ tui_gateway/launch_profile_policy.py | 20 ++- tui_gateway/model_switch.py | 54 ++++--- tui_gateway/server.py | 8 +- 7 files changed, 208 insertions(+), 49 deletions(-) create mode 100644 tests/tui_gateway/test_multi_profile_hosting_transitions.py diff --git a/hermes_cli/config.py b/hermes_cli/config.py index ef2e8406e5..026e910ce3 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -2594,13 +2594,18 @@ def _publish_env_value(key: str, value: Optional[str]) -> None: #77490, #88441. """ try: - from agent.secret_scope import current_secret_scope, is_multiplex_active + from agent.secret_scope import current_secret_scope, serves_routed_profile - scope = current_secret_scope() if is_multiplex_active() else None + scope, routed = current_secret_scope(), serves_routed_profile() except Exception: - scope = None - target = scope if isinstance(scope, dict) else (None if scope is not None else os.environ) - if target is not None: + scope, routed = None, False + # The launch profile's own body runs under a scope snapshot even single-profile (the TUI / + # dashboard launch scope), so a same-request read after the write must see it there too; a + # routed profile's value never reaches the shared process env. + targets = [scope] if isinstance(scope, dict) else [] + if not routed and (scope is None or isinstance(scope, dict)): + targets.append(os.environ) + for target in targets: if value is None: target.pop(key, None) else: diff --git a/hermes_cli/send_cmd.py b/hermes_cli/send_cmd.py index 0f1e9df090..f0a95fe489 100644 --- a/hermes_cli/send_cmd.py +++ b/hermes_cli/send_cmd.py @@ -139,6 +139,9 @@ def _load_hermes_env() -> None: running ``send`` for profile B under its secret scope) it is the installed scope mapping: writing B's ``.env`` into the shared process env would hand every other profile's later reads B's tokens (``gateway.config._getenv`` reads the scope first, so the loader sees the same values either way). + The installed scope is already ``build_profile_secret_scope``'s composition — user ``.env``, then + the profile's external secret sources over it — so it is authoritative as-is; replaying raw + ``.env`` over it would let a stale user value beat the secret-manager one for this request. """ import os try: @@ -146,13 +149,10 @@ def _load_hermes_env() -> None: home = get_hermes_home() except Exception: return - from agent.secret_scope import current_secret_scope, is_multiplex_active, load_env_file + from agent.secret_scope import current_secret_scope, is_multiplex_active scope = current_secret_scope() if is_multiplex_active() else None if isinstance(scope, dict): target: dict = scope - env_path = home / ".env" - if env_path.exists(): - target.update(load_env_file(env_path)) else: target = os.environ env_path = home / ".env" diff --git a/hermes_cli/web_server_profiles.py b/hermes_cli/web_server_profiles.py index 7507898aff..8f8b55ff17 100644 --- a/hermes_cli/web_server_profiles.py +++ b/hermes_cli/web_server_profiles.py @@ -246,8 +246,7 @@ def _config_profile_scope(profile: Optional[str]): Explicit names resolving to the process home retain current-profile semantics. Still enter the requested home so a nested scope cannot retain another profile. """ - from agent.secret_scope import ( - build_profile_secret_scope, is_multiplex_active, reset_secret_scope, set_secret_scope) + from agent.secret_scope import build_profile_secret_scope, reset_secret_scope, set_secret_scope from hermes_cli.env_loader import hydrate_profile_secret_sources from tui_gateway.launch_profile_policy import activate_multi_profile_hosting, launch_secret_scope @@ -261,17 +260,19 @@ def _config_profile_scope(profile: Optional[str]): activate_multi_profile_hosting() hydrate_profile_secret_sources(scoped) # first call may block on the source's fetch secrets = build_profile_secret_scope(scoped) - elif is_multiplex_active(): - secrets = launch_secret_scope(process_home) else: - secrets = None # single-profile dashboard: legacy os.environ precedence (systemd / op-run injection) + # The dashboard's own profile: its launch-env scope (live env + .env while single-profile, so + # systemd / op-run injection keeps resolving; frozen at activation afterwards). Bound even + # before any secondary is served so the request's credential source is decided HERE: a + # concurrent first ``?profile=B`` request flips ``get_secret`` to fail closed mid-request, + # and an unscoped launch request would then raise ``UnscopedSecretError`` on its next read. + secrets = launch_secret_scope(process_home) with (_hermes_home_scope(profile_dir) if profile_dir is not None else nullcontext()): - token = set_secret_scope(secrets) if secrets is not None else None + token = set_secret_scope(secrets) try: yield scoped finally: - if token is not None: - reset_secret_scope(token) + reset_secret_scope(token) # Terminal backend picker rows — GUI counterpart of terminal.backend. Keep in sync with diff --git a/tests/tui_gateway/test_multi_profile_hosting_transitions.py b/tests/tui_gateway/test_multi_profile_hosting_transitions.py new file mode 100644 index 0000000000..7e706447bb --- /dev/null +++ b/tests/tui_gateway/test_multi_profile_hosting_transitions.py @@ -0,0 +1,135 @@ +"""Serve / dashboard profile scopes stay authoritative across the first-secondary transition. + +Three edges of the fail-closed multi-profile host (review of #111620): + +* ``send`` under a routed profile's scope must keep the composed scope (user ``.env`` then external + secret sources) authoritative — replaying raw ``.env`` reversed that precedence for the request; +* a launch-profile body that enters while the process is still single-profile must keep resolving + its own credential after a concurrent first secondary flips ``get_secret`` to fail closed + (``_MULTIPLEX_ACTIVE`` is consulted on every read; the scope decision is made once at entry); +* releasing a runtime scope is per-reset best-effort: a failing terminal reset must not leave the + previous profile's secrets / HERMES_HOME installed for the next body in that context. +""" + +from __future__ import annotations + +import threading +from pathlib import Path + +import pytest + +import tui_gateway.server as server +from tui_gateway import launch_profile_policy as lpp + +A_VAL = "a-only-secret-0001" +B_VAL = "b-only-secret-0002" +ENV_VAL = "systemd-injected-0003" + + +@pytest.fixture +def two_homes(tmp_path, monkeypatch): + root = tmp_path / "hermes_home" + b = root / "profiles" / "b" + b.mkdir(parents=True) + (root / ".env").write_text(f"A_ONLY_TOKEN={A_VAL}\n", encoding="utf-8") + (b / ".env").write_text(f"B_ONLY_TOKEN={B_VAL}\nSHARED_TOKEN=b-dotenv-stale\n", encoding="utf-8") + monkeypatch.setenv("HERMES_HOME", str(root)) + monkeypatch.setenv("A_ONLY_TOKEN", A_VAL) + monkeypatch.setenv("INJECTED_TOKEN", ENV_VAL) # systemd / op run credential injection, no file + monkeypatch.setattr(server, "_hermes_home", root) + monkeypatch.setattr(server, "_served_profile_homes", set()) + monkeypatch.setattr(lpp, "_snapshot", None) + monkeypatch.setattr("agent.secret_scope._MULTIPLEX_ACTIVE", False) + return root, b + + +def test_send_keeps_external_source_value_over_raw_dotenv(two_homes, monkeypatch): + """B's ``.env`` and B's secret manager both define SHARED_TOKEN; the installed scope (manager + wins) survives ``_load_hermes_env`` for the routed ``send``.""" + from hermes_cli import env_loader + from hermes_cli.send_cmd import _load_hermes_env + + root, b = two_homes + # B's secret manager already hydrated for this process (a hydrated home is not re-pulled). + monkeypatch.setattr(env_loader, "_SECRET_SOURCE_VALUES_BY_HOME", + {str(b.resolve()): {"SHARED_TOKEN": "b-manager-fresh"}}) + monkeypatch.setattr(env_loader, "_APPLIED_HOMES", {str(b.resolve())}) + with server._session_profile_runtime_scope({"profile_home": str(b)}): + from agent.secret_scope import current_secret_scope, get_secret + assert get_secret("SHARED_TOKEN") == "b-manager-fresh" + _load_hermes_env() + assert get_secret("SHARED_TOKEN") == "b-manager-fresh" + assert current_secret_scope()["B_ONLY_TOKEN"] == B_VAL + + +def test_launch_body_survives_first_secondary_activation(two_homes): + """Barrier: the launch RPC enters single-profile, B activates on another thread, the launch RPC + resumes and still resolves its env-injected credential instead of raising.""" + from agent.secret_scope import get_secret, is_multiplex_active + + root, b = two_homes + entered, activated = threading.Event(), threading.Event() + seen: dict = {} + + def launch_body(): + with server._session_profile_runtime_scope({"profile_home": None}): + seen["before"] = get_secret("INJECTED_TOKEN") + entered.set() + assert activated.wait(10) + seen["multiplex_now"] = is_multiplex_active() + seen["after"] = get_secret("INJECTED_TOKEN") + seen["b_leak"] = get_secret("B_ONLY_TOKEN") + + t = threading.Thread(target=launch_body) + t.start() + assert entered.wait(10) + assert server._profile_home("b") == b # first secondary: freezes the launch env, flips fail-closed + activated.set() + t.join(10) + assert seen == {"before": ENV_VAL, "multiplex_now": True, "after": ENV_VAL, "b_leak": None} + + +def test_launch_body_survives_first_secondary_activation_on_the_dashboard(two_homes, monkeypatch): + pytest.importorskip("fastapi") + from agent.secret_scope import get_secret + from hermes_cli import web_server_profiles as wsp + + root, b = two_homes + monkeypatch.setattr(wsp, "_resolve_profile_dir", lambda name: b) + entered, activated = threading.Event(), threading.Event() + seen: dict = {} + + def launch_request(): + with wsp._config_profile_scope(None): + seen["before"] = get_secret("INJECTED_TOKEN") + entered.set() + assert activated.wait(10) + seen["after"] = get_secret("INJECTED_TOKEN") + + t = threading.Thread(target=launch_request) + t.start() + assert entered.wait(10) + with wsp._config_profile_scope("b") as scoped: + assert scoped == b and get_secret("B_ONLY_TOKEN") == B_VAL + activated.set() + t.join(10) + assert seen == {"before": ENV_VAL, "after": ENV_VAL} + + +def test_release_resets_every_scope_when_one_reset_fails(two_homes, monkeypatch): + from agent.secret_scope import current_secret_scope + from hermes_constants import get_hermes_home_override + from tools import terminal_scope + + root, b = two_homes + scopes = server._profile_runtime_scope_tokens(str(b)) + assert current_secret_scope() is not None and get_hermes_home_override() == str(b) + + def exploding(_token): + raise RuntimeError("terminal reset blew up") + + monkeypatch.setattr(terminal_scope, "reset_terminal_scope", exploding) + server._release_build_profile_scopes(scopes) # suppresses the re-raised failure + assert current_secret_scope() is None + assert get_hermes_home_override() is None + assert Path(server._hermes_home) == root diff --git a/tui_gateway/launch_profile_policy.py b/tui_gateway/launch_profile_policy.py index c5c7aa3ef3..a84b948ac0 100644 --- a/tui_gateway/launch_profile_policy.py +++ b/tui_gateway/launch_profile_policy.py @@ -47,6 +47,14 @@ def activate_multi_profile_hosting() -> None: set_multiplex_active(True) +def _launch_env() -> Dict[str, str]: + """The launch profile's env: frozen once multiplexing is active; the LIVE process env before + (no secondary has run yet, so it is provably the launch profile's, and freezing it early would + miss values the launch process still bridges at startup).""" + from agent.secret_scope import is_multiplex_active + return capture_launch_env() if is_multiplex_active() else dict(os.environ) + + def launch_terminal_env() -> Dict[str, str]: """The frozen launch ``TERMINAL_*`` overlay for a launch-profile turn's terminal scope. @@ -57,10 +65,14 @@ def launch_terminal_env() -> Dict[str, str]: def launch_secret_scope(launch_home: "str | Path") -> Dict[str, str]: - """The launch profile's secret mapping: its ``.env`` + external sources over the frozen - launch env (systemd / ``op run`` injection survives the fail-closed flip; a secondary never - sees it because its scope is built from its own files only).""" + """The launch profile's secret mapping: its ``.env`` + external sources over the launch env + (systemd / ``op run`` injection survives the fail-closed flip; a secondary never sees it because + its scope is built from its own files only). Bound for EVERY launch-profile body, multiplexing or + not, so the body's credential source is decided once at entry: a request that entered while + single-profile keeps resolving from this mapping after a concurrent first secondary flips + ``get_secret`` to fail closed (``_MULTIPLEX_ACTIVE`` is read on every ``get_secret``, the + scope decision was made at entry).""" from agent.secret_scope import _is_global_env, build_profile_secret_scope - scope = {k: v for k, v in capture_launch_env().items() if not _is_global_env(k)} + scope = {k: v for k, v in _launch_env().items() if not _is_global_env(k)} scope.update(build_profile_secret_scope(Path(launch_home))) return scope diff --git a/tui_gateway/model_switch.py b/tui_gateway/model_switch.py index dd4536aff7..9139bf2f25 100644 --- a/tui_gateway/model_switch.py +++ b/tui_gateway/model_switch.py @@ -51,19 +51,16 @@ def _restore_agent_model_runtime(agent, snapshot: dict | None) -> None: agent.reasoning_config = snapshot["reasoning_config"] -def _launch_profile_scope_needed() -> bool: - """A launch-profile body must run scoped once this process multiplexes (``get_secret`` fails - closed and ambient ``os.environ`` may carry a secondary's residue); a single-profile process - stays unscoped so systemd / ``op run`` credential injection keeps its ``os.environ`` fallthrough.""" - from agent.secret_scope import is_multiplex_active - return is_multiplex_active() - - -def _profile_runtime_scope_tokens(profile_home) -> "_TurnScopes | None": +def _profile_runtime_scope_tokens(profile_home) -> "_TurnScopes": """Bind HERMES_HOME + secret + terminal scope for ``profile_home`` (None = launch profile) and - return the reset tokens; None when nothing needs binding (unscoped single-profile launch body). - The launch profile's scope is its ``.env`` over the env frozen at activation (never live - ``os.environ``: a secondary context may have written to it since, #107422).""" + return the reset tokens. The launch profile's SECRET scope is always bound — its ``.env`` over + the launch env (live while single-profile, frozen at activation afterwards; never live + ``os.environ`` once a secondary context may have written to it, #107422) — so the credential + source is fixed at entry and an in-flight launch body survives a concurrent first-secondary + activation instead of hitting ``UnscopedSecretError`` mid-request. Its terminal policy is bound + only once multiplexing is active: single-profile terminal execution keeps the standalone + ``os.environ`` bridge.""" + from agent.secret_scope import is_multiplex_active scopes = _TurnScopes() if profile_home: home = Path(profile_home) @@ -73,16 +70,18 @@ def _profile_runtime_scope_tokens(profile_home) -> "_TurnScopes | None": secrets = build_profile_secret_scope(home) overlay = None scopes.home = set_hermes_home_override(str(home)) - elif _launch_profile_scope_needed(): + else: # No home override: the launch home IS get_hermes_home() (``_profile_home`` answers None for - # "already the launch profile"); only its secrets + terminal policy need binding. + # "already the launch profile"); only its secrets (+ terminal policy under multiplex) need binding. from tui_gateway.launch_profile_policy import launch_secret_scope, launch_terminal_env home = Path(_hermes_home) secrets = launch_secret_scope(home) + scopes.secret = set_secret_scope(secrets) + if not is_multiplex_active(): + return scopes overlay = launch_terminal_env() - else: - return None - scopes.secret = set_secret_scope(secrets) + if scopes.secret is None: + scopes.secret = set_secret_scope(secrets) # Same terminal policy the gateway binds per turn: a docker-configured profile # must never resolve the launch process's pinned env. Failure → refusal scope. from tools.terminal_scope import install_profile_terminal_scope @@ -91,15 +90,24 @@ def _profile_runtime_scope_tokens(profile_home) -> "_TurnScopes | None": def _release_profile_runtime_scope_tokens(scopes: "_TurnScopes | None") -> None: + """Release terminal → secret → home. Each reset is independent: a failing terminal reset must + not leave the previous profile's secrets / HERMES_HOME installed for the next body in this + context (a fail-open scope leak on the teardown path). The first failure is re-raised after + every scope has been released.""" if scopes is None: return from tools.terminal_scope import reset_terminal_scope - if scopes.terminal is not None: - reset_terminal_scope(scopes.terminal) - if scopes.secret is not None: - reset_secret_scope(scopes.secret) - if scopes.home is not None: - reset_hermes_home_override(scopes.home) + first_error: BaseException | None = None + for token, reset in ((scopes.terminal, reset_terminal_scope), (scopes.secret, reset_secret_scope), + (scopes.home, reset_hermes_home_override)): + if token is None: + continue + try: + reset(token) + except Exception as exc: # noqa: BLE001 — keep releasing the remaining scopes + first_error = first_error or exc + if first_error is not None: + raise first_error @contextlib.contextmanager diff --git a/tui_gateway/server.py b/tui_gateway/server.py index 0325334c12..102bcf3d1a 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -961,13 +961,11 @@ def _wait_agent_for_prompt(session: dict, rid: str, sid: str) -> dict | None: def _bind_build_profile_scopes(profile_home: "str | None") -> "_TurnScopes | None": """Bind a session profile's HERMES_HOME / secret / terminal scopes for an agent build. ``None`` is the - launch profile: unscoped in a single-profile process, its own frozen-env scope once multiplexing is - active (a hosted-room turn for a default member otherwise died at build with ``UnscopedSecretError`` - because the launch profile was treated as "no scope"). Fail-open per scope (the build must not die on + launch profile: its own launch-env secret scope (live env while single-profile, frozen once + multiplexing is active — a hosted-room turn for a default member otherwise died at build with + ``UnscopedSecretError`` because the launch profile was treated as "no scope"). Fail-open per scope (the build must not die on a scope helper); the terminal installer itself fails closed (malformed policy → refusal scope) so _make_agent's terminal probing / cwd hints resolve the routed profile.""" - if not profile_home and not _launch_profile_scope_needed(): - return None scopes = _TurnScopes() with contextlib.suppress(Exception): return _profile_runtime_scope_tokens(profile_home) From db54f5448d7cd13dda34630564153e2edadcfe63 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Wed, 16 Sep 2026 00:08:46 -0700 Subject: [PATCH 11/18] fix(kanban): worker fingerprint carries a boot witness; an uncaptured fingerprint never authorizes a signal #111617 review (andrexibiza P1 #3/#4, kvnloo nit): - worker_started_at persisted only gateway.status.get_process_start_time(): on Linux that is /proc//stat field 22, clock ticks since THIS boot. The threat is a row surviving a reboot, and that counter does not, so an unrelated process on a later boot with the same PID and the same tick value passed _start_times_agree(). The fingerprint is now "|" (boot_id + PID-1 start, the witness the drain marker already uses); both halves must match. Integer values on rows written before this change keep the start-time-only comparison. - A failed capture persisted NULL, which _pid_recycled treats as the legacy pre-fingerprint row and falls back to bare PID existence - a new spawn silently recreated the #89614/ #99558 kill authority. A failed capture now persists UNVERIFIED_WORKER_FINGERPRINT: the claim is held while the PID is live (never released beside it, never SIGTERM/SIGKILLed by timeout, stale-claim, manual reclaim, archive or the terminal reaper) and reclaimed once it is gone. NULL stays legacy-only. - Every tasks UPDATE that nulls worker_pid nulls worker_started_at too (archive_task and the reclaim/timeout/reopen paths): the fingerprint is part of the kill-authority tuple and must not outlive its pid. Live (real sleeper child): reboot-shaped row (same pid, same tick, other boot id) -> reclaimed to ready, child untouched; matching fingerprint -> SIGTERM delivered, exit -15. tests/hermes_cli/test_kanban_worker_pid_fingerprint.py: +2 hostile tests, both red on base. Not changed: the check-then-act window between _pid_recycled and kill (kvnloo P2) is real but needs pidfd_open/pidfd_send_signal (Linux 5.3+) to close atomically; left as the documented residual of "never kills a DETECTED recycled PID". --- hermes_cli/kanban_db.py | 21 ++-- hermes_cli/kanban_db_dispatch.py | 99 ++++++++++++++----- .../test_kanban_worker_pid_fingerprint.py | 71 +++++++++++++ 3 files changed, 157 insertions(+), 34 deletions(-) diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index 04198a301b..95e77e44de 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -890,9 +890,12 @@ CREATE TABLE IF NOT EXISTS tasks ( -- exceeds DEFAULT_FAILURE_LIMIT consecutive non-successes. consecutive_failures INTEGER NOT NULL DEFAULT 0, worker_pid INTEGER, - -- Start-time fingerprint of worker_pid (gateway.status.get_process_start_time) recorded at - -- spawn: liveness and kills require pid AND fingerprint to agree, so a PID recycled after a - -- reboot is never read as our worker or signalled. NULL = legacy row (pre-fingerprint spawn). + -- Restart-stable fingerprint of worker_pid ("|", + -- kanban_db_dispatch._process_fingerprint) recorded at spawn: liveness and kills require pid + -- AND fingerprint to agree, so a PID recycled after a reboot is never read as our worker or + -- signalled. NULL = legacy row (pre-fingerprint spawn); 'unverified' = capture failed at + -- spawn (held while live, never signalled). Column keeps its INTEGER affinity for the + -- start-time-only integer values older rows carry. worker_started_at INTEGER, -- Short excerpt of the most recent failure's error text. last_failure_error TEXT, @@ -2412,7 +2415,7 @@ def release_stale_claims( retry_status = _retry_status_for_run(conn, row["id"]) cur = conn.execute( "UPDATE tasks SET status = ?, claim_lock = NULL, " - "claim_expires = NULL, worker_pid = NULL " + "claim_expires = NULL, worker_pid = NULL, worker_started_at = NULL " "WHERE id = ? AND status = 'running' AND claim_lock IS ? " "AND claim_expires IS NOT NULL AND claim_expires < ?", (retry_status, row["id"], row["claim_lock"], now), @@ -2516,7 +2519,7 @@ def reclaim_task( retry_status = _retry_status_for_run(conn, task_id) cur = conn.execute( "UPDATE tasks SET status = ?, claim_lock = NULL, " - "claim_expires = NULL, worker_pid = NULL " + "claim_expires = NULL, worker_pid = NULL, worker_started_at = NULL " "WHERE id = ? AND status IN ('running', 'ready', 'blocked') " "AND claim_lock IS ?", (retry_status, task_id, prev_lock), ) @@ -3327,7 +3330,7 @@ def request_changes( assignee = COALESCE(?, assignee), claim_lock = NULL, claim_expires = NULL, - worker_pid = NULL + worker_pid = NULL, worker_started_at = NULL WHERE id = ? AND status = 'running' AND current_run_id = ? """, (new_status, implementer, task_id, int(current_run_id)), @@ -3495,7 +3498,7 @@ def reopen_review_task(conn: sqlite3.Connection, task_id: str) -> bool: # consecutive_failures deliberately PRESERVED: review reopen is not # a success signal; only complete_task resets the breaker (#35072). "UPDATE tasks SET status = ?, current_run_id = NULL, " - "claim_lock = NULL, claim_expires = NULL, worker_pid = NULL " + "claim_lock = NULL, claim_expires = NULL, worker_pid = NULL, worker_started_at = NULL " + (", assignee = ?" if implementer else "") + " WHERE id = ? AND status = 'review'", params, @@ -3570,7 +3573,7 @@ def invalidate_descendants_for_parent_reopen( # docstring for why this diverges from reopen_review_task. conn.execute( "UPDATE tasks SET status = 'todo', completed_at = NULL, " - "claim_lock = NULL, claim_expires = NULL, worker_pid = NULL, " + "claim_lock = NULL, claim_expires = NULL, worker_pid = NULL, worker_started_at = NULL, " "current_run_id = NULL, consecutive_failures = 0 WHERE id = ?", (row["id"],), ) entry = { @@ -3688,7 +3691,7 @@ def archive_task(conn: sqlite3.Connection, task_id: str, *, signal_fn=None) -> b prev_pid, prev_lock, prev_started = row["worker_pid"], row["claim_lock"], row["worker_started_at"] cur = conn.execute( "UPDATE tasks SET status = 'archived', " - " claim_lock = NULL, claim_expires = NULL, worker_pid = NULL " + " claim_lock = NULL, claim_expires = NULL, worker_pid = NULL, worker_started_at = NULL " "WHERE id = ? AND status != 'archived'", (task_id,), ) if cur.rowcount != 1: diff --git a/hermes_cli/kanban_db_dispatch.py b/hermes_cli/kanban_db_dispatch.py index a99382e79c..f8e32c1c61 100644 --- a/hermes_cli/kanban_db_dispatch.py +++ b/hermes_cli/kanban_db_dispatch.py @@ -295,20 +295,52 @@ def _pid_alive(pid: Optional[int]) -> bool: return True -def _worker_alive(pid: Optional[int], started_at: Optional[int]) -> bool: - """True when ``pid`` is live AND is still the worker we spawned. ``started_at`` is the start-time - fingerprint recorded by ``_set_worker_pid``; after a reboot (or any PID recycle) an unrelated - process can own the number, so bare existence is never enough to extend a claim or to signal. - A legacy row without a fingerprint keeps the existence answer: killing it is the pre-fingerprint - behaviour and the row is rewritten with a fingerprint on its next spawn.""" - return _kb._pid_alive(pid) and not _pid_recycled(pid, started_at) +# ``worker_started_at`` value for a spawn whose fingerprint could not be captured. Distinct from the +# NULL legacy row (pre-fingerprint spawn): such a worker is held (its claim is never released beside +# the live PID) but NEVER signalled — missing process identity is refusal, not permission (#99558). +UNVERIFIED_WORKER_FINGERPRINT = "unverified" -def _pid_recycled(pid: Optional[int], started_at: Optional[int]) -> bool: +def _process_fingerprint(pid: int) -> Optional[str]: + """Restart-stable identity of a live process: ``"|"``. The start + time alone (``/proc//stat`` field 22 on Linux) is clock ticks since THIS boot, so a row that + survives a reboot could match an unrelated process with the same PID and the same tick value; + ``gateway.drain_control.current_instantiation_epoch`` (``boot_id`` + PID-1 start) changes on every + reboot / container recreate, so the composed value never survives one. ``None`` when unreadable.""" + from gateway.drain_control import current_instantiation_epoch + from gateway.status import get_process_start_time + start = get_process_start_time(int(pid)) + if start is None: + return None + return f"{current_instantiation_epoch()}|{start}" + + +def _worker_alive(pid: Optional[int], started_at) -> bool: + """True when ``pid`` is live AND is still the worker we spawned. ``started_at`` is the fingerprint + recorded by ``_set_worker_pid``; after a reboot (or any PID recycle) an unrelated process can own + the number, so bare existence is never enough to extend a claim or to signal. A legacy row without + a fingerprint keeps the existence answer: killing it is the pre-fingerprint behaviour and the row is + rewritten with a fingerprint on its next spawn. An UNVERIFIED spawn also keeps the existence answer + (a claim is never released beside a possibly-live worker) but ``_terminate_reclaimed_worker`` + refuses to signal it.""" + if not _kb._pid_alive(pid): + return False + if started_at == UNVERIFIED_WORKER_FINGERPRINT: + return True + return not _pid_recycled(pid, started_at) + + +def _pid_recycled(pid: Optional[int], started_at) -> bool: """True when a live ``pid`` is NOT the process fingerprinted at spawn (or the fingerprint can no - longer be read). Signalling it would hit a stranger. ``None`` fingerprint = legacy row, never recycled.""" + longer be read). Signalling it would hit a stranger. ``None`` fingerprint = legacy row, never + recycled; the UNVERIFIED marker is always foreign. An integer fingerprint (rows written before the + boot witness was added) compares the start time only.""" if started_at is None or not pid: return False + if started_at == UNVERIFIED_WORKER_FINGERPRINT: + return True + if isinstance(started_at, str) and "|" in started_at: + return _process_fingerprint(int(pid)) != started_at from gateway.status import _start_times_agree, get_process_start_time current = get_process_start_time(int(pid)) if current is None: @@ -350,11 +382,14 @@ def _terminate_reclaimed_worker( claim_lock: Optional[str], *, signal_fn=None, - started_at: Optional[int] = None, + started_at=None, ) -> dict[str, Any]: """Best-effort host-local worker termination for reclaim paths. ``started_at`` is the spawn-time fingerprint: when the live process no longer matches it, the PID was recycled and nothing is - signalled — the worker is gone, which is what the reclaim wanted (``terminated`` = True).""" + signalled — the worker is gone, which is what the reclaim wanted (``terminated`` = True). An + UNVERIFIED spawn (fingerprint capture failed) that is still live is never signalled either, but + it is reported as surviving (``signal_refused``) so the reclaim holds the claim instead of + spawning a duplicate beside it.""" info: dict[str, Any] = { "prev_pid": int(pid) if pid else None, "host_local": False, @@ -371,6 +406,11 @@ def _terminate_reclaimed_worker( kill = _kill_fn(signal_fn) if kill is None: return info + if started_at == UNVERIFIED_WORKER_FINGERPRINT: + # Never signal by bare number: a dead PID is "gone" (reclaim proceeds), a live one is held. + info["signal_refused"] = True + info["terminated"] = not _kb._pid_alive(pid) + return info if _kb._pid_alive(pid) and _pid_recycled(pid, started_at): info["terminated"] = True info["pid_recycled"] = True @@ -429,9 +469,11 @@ def reap_terminal_workers(conn: sqlite3.Connection, *, signal_fn=None) -> list[s def _reap_terminal_worker_row(conn, row, host_prefix: str, signal_fn, reaped: list[str]) -> None: - pid, fingerprint = int(row["worker_pid"]), int(row["worker_started_at"]) + pid, fingerprint = int(row["worker_pid"]), row["worker_started_at"] if pid == os.getpid() or not str(row["claim_lock"] or "").startswith(host_prefix): return + if fingerprint == UNVERIFIED_WORKER_FINGERPRINT and _kb._pid_alive(pid): + return # unproven identity: never signalled; its evidence is cleared once the pid is gone alive = _worker_alive(pid, fingerprint) termination = None if alive: @@ -463,8 +505,8 @@ def _worker_survived_termination(termination: dict) -> bool: through to the normal release path since we cannot manage that worker anyway. """ return bool( - termination.get("termination_attempted") - and termination.get("host_local") + termination.get("host_local") + and (termination.get("termination_attempted") or termination.get("signal_refused")) and not termination.get("terminated") ) @@ -576,6 +618,12 @@ def enforce_max_runtime(conn: sqlite3.Connection, *, signal_fn=None) -> list[str pid = int(row["worker_pid"]) tid = row["id"] started_at = _kb._row_get(row, "worker_started_at") + if started_at == UNVERIFIED_WORKER_FINGERPRINT and _kb._pid_alive(pid): + # Fingerprint capture failed at spawn: we cannot prove this live PID is our worker, so + # it is neither signalled nor released beside (duplicate). It is reclaimed once it exits. + _kb._log.warning("kanban: task %s worker pid %s exceeded max runtime but has no verified " + "identity; not signalled", tid, pid) + continue # SIGTERM then SIGKILL after 5 s grace; workers wanting a cleaner # shutdown install their own SIGTERM handler. A recycled PID (fingerprint # mismatch) is never signalled: the worker is already gone. @@ -594,7 +642,7 @@ def enforce_max_runtime(conn: sqlite3.Connection, *, signal_fn=None) -> list[str retry_status = _kb._retry_status_for_run(conn, tid) cur = conn.execute( "UPDATE tasks SET status = ?, claim_lock = NULL, " - "claim_expires = NULL, worker_pid = NULL, " + "claim_expires = NULL, worker_pid = NULL, worker_started_at = NULL, " "last_heartbeat_at = NULL " "WHERE id = ? AND status = 'running' " " AND worker_pid = ? AND claim_lock IS ?", @@ -696,7 +744,7 @@ def detect_stale_running( retry_status = _kb._retry_status_for_run(conn, tid) cur = conn.execute( "UPDATE tasks SET status = ?, claim_lock = NULL, " - "claim_expires = NULL, worker_pid = NULL, " + "claim_expires = NULL, worker_pid = NULL, worker_started_at = NULL, " "last_heartbeat_at = NULL " "WHERE id = ? AND status = 'running' " " AND claim_lock IS ?", @@ -761,7 +809,7 @@ def reconcile_orphaned_running(conn: sqlite3.Connection) -> list[str]: with _kb.write_txn(conn): cur = conn.execute( "UPDATE tasks SET status = 'ready', claim_lock = NULL, " - "claim_expires = NULL, worker_pid = NULL, " + "claim_expires = NULL, worker_pid = NULL, worker_started_at = NULL, " "last_heartbeat_at = NULL " "WHERE id = ? AND status = 'running' " " AND claim_lock IS ? AND claim_expires IS ?", @@ -1015,7 +1063,7 @@ def _reclaim_dead_workers(conn: sqlite3.Connection, board: Optional[str] = None) dead.event_payload["retry_status"] = retry_status cur = conn.execute( "UPDATE tasks SET status = ?, claim_lock = NULL, " - "claim_expires = NULL, worker_pid = NULL " + "claim_expires = NULL, worker_pid = NULL, worker_started_at = NULL " "WHERE id = ? AND status = 'running' " " AND worker_pid = ? AND claim_lock IS ?", (retry_status, row["id"], pid, row["claim_lock"]), @@ -1221,7 +1269,7 @@ def _record_task_failure( # Spawn path: restore the claimed source phase + clear claim. conn.execute( "UPDATE tasks SET status = ?, claim_lock = NULL, " - "claim_expires = NULL, worker_pid = NULL, " + "claim_expires = NULL, worker_pid = NULL, worker_started_at = NULL, " "consecutive_failures = ?, last_failure_error = ? " "WHERE id = ? AND status = 'running'", (retry_status, failures, error, task_id), @@ -1249,7 +1297,7 @@ def _record_task_failure( # state; the timeout/crash path already did. conn.execute( "UPDATE tasks SET status = 'blocked', " - + ("claim_lock = NULL, claim_expires = NULL, worker_pid = NULL, " + + ("claim_lock = NULL, claim_expires = NULL, worker_pid = NULL, worker_started_at = NULL, " if release_claim else "") + "consecutive_failures = ?, last_failure_error = ? " "WHERE id = ? AND status IN ('running', 'ready', 'review')", @@ -1283,11 +1331,12 @@ def _record_task_failure( def _set_worker_pid(conn: sqlite3.Connection, task_id: str, pid: int) -> None: - """Record the spawned child's pid + its start-time fingerprint, and emit a ``spawned`` event - carrying them. The fingerprint is what lets every later liveness/kill decision tell OUR worker - from a process that recycled the PID after a reboot.""" - from gateway.status import get_process_start_time - started_at = get_process_start_time(int(pid)) + """Record the spawned child's pid + its restart-stable fingerprint (``_process_fingerprint``), and + emit a ``spawned`` event carrying them. The fingerprint is what lets every later liveness/kill + decision tell OUR worker from a process that recycled the PID after a reboot. A failed capture is + persisted as ``UNVERIFIED_WORKER_FINGERPRINT``, never NULL: NULL is the legacy pre-fingerprint row + whose bare-PID kill authority a new spawn must not inherit.""" + started_at = _process_fingerprint(int(pid)) or UNVERIFIED_WORKER_FINGERPRINT with _kb.write_txn(conn): conn.execute("UPDATE tasks SET worker_pid = ?, worker_started_at = ? WHERE id = ?", (int(pid), started_at, task_id)) diff --git a/tests/hermes_cli/test_kanban_worker_pid_fingerprint.py b/tests/hermes_cli/test_kanban_worker_pid_fingerprint.py index a7fc571987..d49d54400c 100644 --- a/tests/hermes_cli/test_kanban_worker_pid_fingerprint.py +++ b/tests/hermes_cli/test_kanban_worker_pid_fingerprint.py @@ -78,3 +78,74 @@ def test_matching_fingerprint_keeps_the_live_worker(board): conn.execute("UPDATE tasks SET max_runtime_seconds = 1 WHERE id = ?", (tid,)) kbd.enforce_max_runtime(conn, signal_fn=lambda pid, sig: killed.append((pid, sig))) assert killed and killed[0] == (os.getpid(), signal.SIGTERM) + + +def test_same_pid_and_start_tick_on_another_boot_is_foreign(board, monkeypatch): + """A row that survived a reboot: the PID AND the boot-relative start tick both match a process on + this boot (the Linux start time is clock ticks since boot, so that recurs), but the persisted + instantiation epoch does not. The worker is foreign: claim released, zero signals.""" + from gateway import drain_control + + conn = board + killed = [] + live_fingerprint = kbd._process_fingerprint(os.getpid()) + assert live_fingerprint is not None and live_fingerprint.split("|", 1)[1] == str( + __import__("gateway.status", fromlist=["x"]).get_process_start_time(os.getpid())) + tid = _claimed_running(conn, pid=os.getpid(), started_at=live_fingerprint, max_runtime=1) + assert kbd._worker_alive(os.getpid(), live_fingerprint) is True + + # Same PID, same start tick, different boot identity. + other_boot = "deadbeef-boot:1|" + live_fingerprint.split("|", 1)[1] + with kb.write_txn(conn): + conn.execute("UPDATE tasks SET worker_started_at = ? WHERE id = ?", (other_boot, tid)) + assert kbd._worker_alive(os.getpid(), other_boot) is False + assert tid in kbd.enforce_max_runtime(conn, signal_fn=lambda pid, sig: killed.append((pid, sig))) + assert killed == [] + task = kb.get_task(conn, tid) + assert task.status == "ready" and task.worker_pid is None + + # The same value re-derived on THIS boot still identifies our worker (the witness is stable + # within a boot, unlike the recorded epoch of a previous one). + drain_control.current_instantiation_epoch.cache_clear() + assert kbd._process_fingerprint(os.getpid()) == live_fingerprint + + +def test_unverified_fingerprint_capture_never_authorizes_a_signal(board, monkeypatch): + """Fingerprint capture fails for a new spawn: the row is NOT a legacy NULL row. A live PID under + it is never SIGTERM/SIGKILLed by any reclaim/timeout path, and the claim is held (not released + beside the live process); once the PID is gone the claim is reclaimed normally.""" + import gateway.status as status + + conn = board + killed = [] + monkeypatch.setattr(status, "_get_process_start_time", lambda pid: None) + tid = kb.create_task(conn, title="job", assignee="worker", max_runtime_seconds=1) + kb.claim_task(conn, tid) + kbd._set_worker_pid(conn, tid, os.getpid()) + row = conn.execute("SELECT worker_started_at FROM tasks WHERE id = ?", (tid,)).fetchone() + assert row["worker_started_at"] == kbd.UNVERIFIED_WORKER_FINGERPRINT + monkeypatch.undo() + old = int(time.time()) - 3600 + with kb.write_txn(conn): + conn.execute("UPDATE tasks SET started_at = ?, claim_expires = ? WHERE id = ?", (old, old, tid)) + conn.execute("UPDATE task_runs SET started_at = ? WHERE id = (SELECT current_run_id FROM tasks WHERE id = ?)", + (old, tid)) + + sig = lambda pid, s: killed.append((pid, s)) # noqa: E731 + assert kbd.enforce_max_runtime(conn, signal_fn=sig) == [] + assert kb.release_stale_claims(conn, signal_fn=sig) == 0 + assert killed == [] + assert kb.get_task(conn, tid).status == "running" + # An explicit operator reclaim releases the claim (human override) but still sends nothing. + assert kb.reclaim_task(conn, tid, reason="operator", signal_fn=sig) is True + assert killed == [] + + # The process is gone (a dead PID): the row is reclaimed like any dead worker, still no signal. + tid2 = kb.create_task(conn, title="job2", assignee="worker") + kb.claim_task(conn, tid2) + with kb.write_txn(conn): + conn.execute("UPDATE tasks SET worker_pid = ?, worker_started_at = ?, claim_expires = ? WHERE id = ?", + (os.getpid(), kbd.UNVERIFIED_WORKER_FINGERPRINT, old, tid2)) + monkeypatch.setattr(kb, "_pid_alive", lambda pid: False) + assert kb.release_stale_claims(conn, signal_fn=sig) == 1 + assert killed == [] and kb.get_task(conn, tid2).status == "ready" From 92d64465bf2560e7c4d5f715f0b656c2139d0de0 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Wed, 16 Sep 2026 00:09:22 -0700 Subject: [PATCH 12/18] docs(multiplex): routed-child credential rule and the frozen launch env restart requirement kvnloo (#111620 review) asked for the operator-visible statement that once a serve / dashboard process hosts a second profile, the launch profile's env-only credentials are frozen at activation and a rotation in the process env needs a restart. Also states the routed-child rule with and without the multiplex flag (#111617). Adds encoding='utf-8' to the probe child in the child-env authority test (windows-footguns lint). --- tests/tui_gateway/test_served_profile_child_env_authority.py | 2 +- website/docs/user-guide/multi-profile-gateways.md | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/tui_gateway/test_served_profile_child_env_authority.py b/tests/tui_gateway/test_served_profile_child_env_authority.py index e1303003cf..9d6062c8e4 100644 --- a/tests/tui_gateway/test_served_profile_child_env_authority.py +++ b/tests/tui_gateway/test_served_profile_child_env_authority.py @@ -92,7 +92,7 @@ def test_real_child_observes_only_the_routed_profile(homes): finally: reset_hermes_home_override(token) probe = "import json,os;print(json.dumps({k:os.environ.get(k) for k in ('HERMES_HOME','A_MARKER','B_MARKER','OPENAI_API_KEY')}))" - out = subprocess.run([sys.executable, "-c", probe], env=env, capture_output=True, text=True, timeout=60) + out = subprocess.run([sys.executable, "-c", probe], env=env, capture_output=True, text=True, encoding="utf-8", timeout=60) seen = json.loads(out.stdout.strip().splitlines()[-1]) assert seen == {"HERMES_HOME": str(b), "A_MARKER": None, "B_MARKER": "b", "OPENAI_API_KEY": None} assert Path(seen["HERMES_HOME"]) == b diff --git a/website/docs/user-guide/multi-profile-gateways.md b/website/docs/user-guide/multi-profile-gateways.md index 2398580b58..a7bbf5f5df 100644 --- a/website/docs/user-guide/multi-profile-gateways.md +++ b/website/docs/user-guide/multi-profile-gateways.md @@ -448,6 +448,8 @@ profile and never shares with the default or any sibling: | MCP discovery in the Desktop/dashboard backend | Once per served profile home | A profile selected after another has already built an agent still discovers its own `mcp_servers` | | MCP connections in the Desktop/dashboard backend and the per-profile cron ticker | Keyed per served profile even with `gateway.multiplex_profiles` off — same rule as the multiplexer | A same-named `mcp_servers` entry with other credentials is its own connection; a served profile never calls a server as another profile | | Dashboard actions (`hermes -p …` spawned by the Desktop/dashboard) | A scrubbed child env pinned to that profile's `HERMES_HOME` | The child loads its own `.env`; the dashboard profile's tokens and ports are not inherited | +| Every child that acts for a served profile (slash worker, Bot Chat delivery, A2A forward, `key_cmd` helper, browser driver) | That profile's own `.env` + secret sources over a credential-scrubbed base — with or without `gateway.multiplex_profiles` (the Desktop/dashboard `?profile=` route counts) | Absent from the child — a key that reached the launch process only through systemd / Compose / the shell is never inherited by another profile's child | +| The launch (default) profile's own credentials in a `hermes serve` / dashboard process that also serves another profile | Its `.env` + secret sources over the process env **frozen the moment the first other profile is served**; not re-read afterwards | A credential rotated only in the process env (`systemctl set-environment`, a refreshed `op run` wrapper that did not re-exec) is not picked up until the process restarts — put rotating keys in `.env` or a secret source, or restart after rotating | | Cron `.env` tuning (`HERMES_CRON_TIMEOUT`, `HERMES_MODEL` fallback, `HERMES_CRON_MAX_PARALLEL`, prefill file), worker / Bot Chat child env | The profile's own `.env`; children never inherit the default profile's `.env` settings or bridged `TERMINAL_*` policy | Cron defaults / model refusal, exactly as a standalone `hermes -p gateway run` | | Kanban workers and notifications for a profile's tasks | The assignee's `.env` + `config.yaml` (toolset pin, terminal backend, media policy, display language) | — | | `/loop` ticks, `background_process_notifications` gate, `notice_delivery`, background-process checkpoint recovery | The owning profile's `state.db` / `config.yaml` / `processes.json` | — | From 287c56e95afe5c528beacb7ca8f7ef0ad6216f2a Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Wed, 16 Sep 2026 00:22:39 -0700 Subject: [PATCH 13/18] test(kanban): archive-terminates-worker fixture records a verified spawn fingerprint _set_worker_pid on a fake PID (54321) now persists the 'unverified' marker, under which the archive path correctly refuses to signal; the test pins the verified-spawn behaviour, so it stubs the fingerprint capture. --- tests/hermes_cli/test_kanban_db.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/hermes_cli/test_kanban_db.py b/tests/hermes_cli/test_kanban_db.py index 65c9f0ed59..f0a4bb9455 100644 --- a/tests/hermes_cli/test_kanban_db.py +++ b/tests/hermes_cli/test_kanban_db.py @@ -1871,6 +1871,8 @@ def test_archive_running_task_terminates_worker(kanban_home, monkeypatch): t = kb.create_task(conn, title="x", assignee="a") host = kb._claimer_id().split(":", 1)[0] kb.claim_task(conn, t, claimer=f"{host}:worker") + # A verified spawn: an uncaptured fingerprint would (correctly) refuse the signal. + monkeypatch.setattr(kbd, "_process_fingerprint", lambda _pid: "boot:1|777") kbd._set_worker_pid(conn, t, 54321) monkeypatch.setattr(kb, "_pid_alive", lambda _pid: False) From 65b622da0d0dad72d102e8a43a7f5d8ed573bec9 Mon Sep 17 00:00:00 2001 From: brooklyn! Date: Fri, 11 Sep 2026 22:57:59 -0700 Subject: [PATCH 14/18] fix(desktop): transcript directives survive markdown, and the guide keeps its reasoning to itself MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A handoff directive whose brief read as markdown (*by week*, a_b c_d, ~/x) split the paragraph into element children, so the card never claimed it and the raw ::onboarding{task="…"} line painted as the user's own message. Directive lines are now backslash-shielded in preprocessMarkdown, next to the inline-code and math shields, and reach the renderer as one text node. The guide's reasoning is it reading its own runbook ("Now step 4: offer the tour with ::ask"); shown under the greeting it breaks the conversation the guide is trying to have. Reasoning disclosures stay hidden on the guide thread only. --- .../assistant-ui/thread/message-parts.tsx | 7 +- .../markdown-preprocess.directives.test.ts | 65 +++++++++++++++++++ apps/desktop/src/lib/markdown-preprocess.ts | 31 ++++++++- 3 files changed, 101 insertions(+), 2 deletions(-) create mode 100644 apps/desktop/src/lib/markdown-preprocess.directives.test.ts diff --git a/apps/desktop/src/components/assistant-ui/thread/message-parts.tsx b/apps/desktop/src/components/assistant-ui/thread/message-parts.tsx index d0351d5721..944c809c47 100644 --- a/apps/desktop/src/components/assistant-ui/thread/message-parts.tsx +++ b/apps/desktop/src/components/assistant-ui/thread/message-parts.tsx @@ -20,6 +20,7 @@ import { formatElapsed, useElapsedSeconds, useMeasuredDuration } from '@/compone import { ActivityTimerText } from '@/components/chat/activity-timer-text' import { GeneratedImage } from '@/components/chat/generated-image-result' import { SCAFFOLD_LABEL_CLASS, SCAFFOLD_META_CLASS, ScaffoldRow } from '@/components/chat/scaffold-row' +import { useOnboardingChatActive } from '@/components/onboarding-chat/assembly' import { useI18n } from '@/i18n' import { connectorCalls, mcpTargets } from '@/lib/connector-tools' import { generatedImageFromResult } from '@/lib/generated-images' @@ -286,6 +287,10 @@ const ReasoningAccordionGroup: FC<{ children?: ReactNode; endIndex: number; star }) => { const messageId = useAuiState(s => s.message.id) const messageRunning = useAuiState(s => s.message.status?.type === 'running') + // The guide's reasoning is it reading its own runbook ("Now step 4: offer + // the tour with ::ask"), and a first-time user reading that alongside the + // greeting breaks the one conversation the guide is trying to have. + const guidedChat = useOnboardingChatActive() const pending = useAuiState( s => @@ -324,7 +329,7 @@ const ReasoningAccordionGroup: FC<{ children?: ReactNode; endIndex: number; star }, undefined) ) - if (!hasContent) { + if (!hasContent || guidedChat) { return null } diff --git a/apps/desktop/src/lib/markdown-preprocess.directives.test.ts b/apps/desktop/src/lib/markdown-preprocess.directives.test.ts new file mode 100644 index 0000000000..7e8e02708a --- /dev/null +++ b/apps/desktop/src/lib/markdown-preprocess.directives.test.ts @@ -0,0 +1,65 @@ +import remarkGfm from 'remark-gfm' +import remarkParse from 'remark-parse' +import { unified } from 'unified' +import { describe, expect, it } from 'vitest' + +import { preprocessMarkdown } from './markdown-preprocess' + +/** + * A transcript directive (`::onboarding{...}`) is a card only while the parser + * hands the paragraph over as ONE text node; the moment an attribute value + * reads as markdown the paragraph splits into element children and the raw + * directive paints as the user's message. These are the shapes that leaked. + */ + +// The paragraph's children as the renderer will see them, after preprocess. +function paragraphChildren(markdown: string) { + const tree = unified().use(remarkParse).use(remarkGfm).parse(preprocessMarkdown(markdown)) as { + children: { children?: { type: string; value?: string }[] }[] + } + + return tree.children[0]?.children ?? [] +} + +const briefs = [ + 'Track monarch sightings *by week* and plot them', + 'Track a_b and c_d values across runs', + 'Sync ~/notes to ~/backup nightly', + 'Wrap the `run` command with retries', + 'Compare and snapshots', + 'Parse [id] tokens and \\n escapes' +] + +describe('preprocessMarkdown / directive lines', () => { + it.each(briefs)('stays one text node with brief: %s', brief => { + const line = `::onboarding{step="handoff" task="Tracker" brief="${brief}" plan="build"}` + const children = paragraphChildren(line) + + expect(children.map(child => child.type)).toEqual(['text']) + // The parser eats the escapes, so the directive arrives as written. + expect(children[0]?.value).toBe(line) + }) + + it('leaves the surrounding prose to markdown', () => { + const text = 'Perfect, that is *all* I needed.\n\n::onboarding{step="handoff" task="T" brief="a_b" plan="build"}' + + const tree = unified().use(remarkParse).use(remarkGfm).parse(preprocessMarkdown(text)) as { + children: { children?: { type: string }[] }[] + } + + expect(tree.children[0]?.children?.map(child => child.type)).toEqual(['text', 'emphasis', 'text']) + expect(tree.children[1]?.children?.map(child => child.type)).toEqual(['text']) + }) + + it('does not touch a directive inside a code fence', () => { + const text = '```\n::onboarding{step="handoff" brief="a_b"}\n```' + + expect(preprocessMarkdown(text)).toBe(text) + }) + + it('does not touch prose that merely contains ::', () => { + const text = 'Use std::vector for *speed*.' + + expect(preprocessMarkdown(text)).toBe(text) + }) +}) diff --git a/apps/desktop/src/lib/markdown-preprocess.ts b/apps/desktop/src/lib/markdown-preprocess.ts index dc5d669ef3..ec95338e63 100644 --- a/apps/desktop/src/lib/markdown-preprocess.ts +++ b/apps/desktop/src/lib/markdown-preprocess.ts @@ -56,6 +56,15 @@ const CITATION_MARKER_RE = /(?<=[\p{L}\p{N})\].,!?:;"'”’])\[(?:\d+(?:\s*,\s* // char class excludes `)`/whitespace, matching how LLMs actually emit these. const FILE_LINK_RE = /(?[^\]\n]+)\]\((??)\)/gi +// A transcript directive on its own line: `::name{...}`. Attribute values are +// prose the model wrote (a task brief, a question) and read as markdown to the +// parser — `*by week*` becomes , `a_b c_d` becomes , `~/x` opens +// strikethrough. Any of those splits the paragraph into element children, the +// directive stops being text-only, and the raw line paints as the user's +// message. Same shape as lib/transcript-directives.ts DIRECTIVE_RE. +const DIRECTIVE_LINE_RE = /^([ \t]*)(::[a-z][a-z0-9-]{0,63}\{[^{}\n]{0,1024}\})[ \t]*$/gm +const MARKDOWN_INLINE_META_RE = /[\\`*_~[\]<>]/g + /** * Returns true when `body` contains a line that's exactly `marker` (modulo * leading/trailing horizontal whitespace) — i.e. an unambiguous close fence @@ -203,6 +212,22 @@ function rewriteProseSegment(segment: string): string { ) } +/** + * Backslash-escape markdown inline syntax inside directive lines so the parser + * yields one text node. The escapes are consumed by the parser, so the + * directive the renderer sees is byte-for-byte what the model wrote. + */ +export function shieldDirectiveLines(text: string): string { + if (!text.includes('::')) { + return text + } + + return text.replace( + DIRECTIVE_LINE_RE, + (_match, indent: string, directive: string) => indent + directive.replace(MARKDOWN_INLINE_META_RE, '\\$&') + ) +} + /** * Apply the prose rewrites to visible prose only. * @@ -641,7 +666,11 @@ export function preprocessMarkdown(text: string): string { // blocks stay intact. The HTML-depth clamp belongs here for the same // reason: a fenced block renders as code and never reaches rehype-raw, // so escaping tags inside one would corrupt the listing for nothing. - return clampHtmlNestingDepth(normalizeVisibleProse(stripPreviewTargets(normalizeProseMath(part)))) + // Directive lines are shielded last, after the prose rewrites have had + // their look, so nothing re-introduces markdown into them. + return shieldDirectiveLines( + clampHtmlNestingDepth(normalizeVisibleProse(stripPreviewTargets(normalizeProseMath(part)))) + ) }) .join('') } From c7580f0e06fa40ed5bbfbac860d465d644a9f59e Mon Sep 17 00:00:00 2001 From: brooklyn! Date: Wed, 16 Sep 2026 00:35:51 -0700 Subject: [PATCH 15/18] feat(desktop): FanMenu primitive and a floating Button variant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One hub control that fans its sibling toggles out on hover — a column, a row split around the hub, or an arc — with the discs portalled past any overflow-hidden parent and re-anchored on scroll and resize. Hover state, geometry and open/close timing live in the primitive so a consumer only re-renders when its own items change. A document-level pointer watchdog closes the fan when enter/leave is skipped: a disc that goes disabled under the pointer (a toggle turning pending right after its click) stops receiving pointer events, so its pointerleave never fires and the fan stayed open. Discs wear the new Button `floating` variant off (popover fill + shadow-md, glyph-only hover) and `default` on, so a toggled state is an opaque primary disc rather than a translucent tint. --- apps/desktop/DESIGN.md | 12 +- apps/desktop/src/components/ui/button.tsx | 5 + .../src/components/ui/fan-menu.test.ts | 56 +++ apps/desktop/src/components/ui/fan-menu.tsx | 397 ++++++++++++++++++ 4 files changed, 467 insertions(+), 3 deletions(-) create mode 100644 apps/desktop/src/components/ui/fan-menu.test.ts create mode 100644 apps/desktop/src/components/ui/fan-menu.tsx diff --git a/apps/desktop/DESIGN.md b/apps/desktop/DESIGN.md index e22dfde9e4..7335f3ce58 100644 --- a/apps/desktop/DESIGN.md +++ b/apps/desktop/DESIGN.md @@ -118,9 +118,10 @@ do **not** pass `h-*`, `px-*`, `py-*`, or icon-size overrides. **Variants:** `default` (primary), `destructive`, `secondary` (soft fill — the default non-primary look), `outline` (transparent + 1px inset ring, no -fill/shadow), `ghost`, `link`, `text` (boxless quiet inline — "Cancel", -"Clear"), `textStrong` (bold underlined inline affordance — "Change", -"Open logs"). +fill/shadow), `ghost`, `floating` (a control loose from any surface — opaque +popover fill + `shadow-md`, hover lifts the glyph only), `link`, `text` +(boxless quiet inline — "Cancel", "Clear"), `textStrong` (bold underlined +inline affordance — "Change", "Open logs"). **Sizes:** `default`, `xs`, `sm`, `lg`, `inline` (flush, zero box — for buttons that sit inside a heading/sentence; replaces `h-auto px-0 py-0`), `micro` @@ -196,6 +197,11 @@ blurred backdrop. (color mode, tool-call display, usage period). Replaces radio piles and pill rows. - **`Switch`** (`size="xs"`) — bare, with `aria-label`. No bordered text wrapper. +- **`FanMenu`** (`src/components/ui/fan-menu.tsx`) — one hub control that + fans sibling toggles out on hover: `direction` `vertical` | `horizontal` + (split around the hub) | `arc`. Discs are `Button` `floating` off / + `default` on; tips anchor left by default. Use it where a row of rarely + touched toggles is costing input width (the composer's voice controls). ## Layout diff --git a/apps/desktop/src/components/ui/button.tsx b/apps/desktop/src/components/ui/button.tsx index 444ea564c3..fdbb64b8c0 100644 --- a/apps/desktop/src/components/ui/button.tsx +++ b/apps/desktop/src/components/ui/button.tsx @@ -26,6 +26,11 @@ const buttonVariants = cva( secondary: 'bg-(--ui-bg-quaternary) text-(--ui-text-primary) hover:bg-(--chrome-action-hover) hover:text-(--ui-text-primary)', ghost: 'text-(--ui-text-secondary) hover:bg-(--chrome-action-hover) hover:text-(--ui-text-primary)', + // A control floating free of any surface (fan-menu discs, detached + // chips): the menu/popover treatment — opaque popover fill + the + // shared `shadow-md` ring-and-drop. Hover only lifts the glyph; a fill + // change on a lone disc reads as a toggle flipping. + floating: 'bg-popover text-(--ui-text-secondary) shadow-md hover:text-(--ui-text-primary)', link: `text-primary underline-offset-4 decoration-current/20 hover:underline ${TEXT_ACTION_ICON}`, // Boxless inline-text action (no bg/border). Quiet by default — reads as // muted label text, underlines on hover (e.g. "Cancel", "Clear"). diff --git a/apps/desktop/src/components/ui/fan-menu.test.ts b/apps/desktop/src/components/ui/fan-menu.test.ts new file mode 100644 index 0000000000..62808177aa --- /dev/null +++ b/apps/desktop/src/components/ui/fan-menu.test.ts @@ -0,0 +1,56 @@ +import { describe, expect, it } from 'vitest' + +import { fanOffsets } from './fan-menu' + +const STEP = 28 + +const dist = (a: { x: number; y: number }, b: { x: number; y: number }) => Math.hypot(a.x - b.x, a.y - b.y) + +describe('fanOffsets', () => { + it('stacks a column straight up, one step apart', () => { + expect(fanOffsets('vertical', 3, STEP)).toEqual([ + { x: 0, y: -STEP }, + { x: 0, y: -2 * STEP }, + { x: 0, y: -3 * STEP } + ]) + }) + + it('splits a row around the hub in reading order, extra one on the right', () => { + expect(fanOffsets('horizontal', 2, STEP)).toEqual([ + { x: -STEP, y: 0 }, + { x: STEP, y: 0 } + ]) + expect(fanOffsets('horizontal', 3, STEP)).toEqual([ + { x: -STEP, y: 0 }, + { x: STEP, y: 0 }, + { x: 2 * STEP, y: 0 } + ]) + expect(fanOffsets('horizontal', 4, STEP).map(p => p.x)).toEqual([-2 * STEP, -STEP, STEP, 2 * STEP]) + }) + + // Neighbours never touch in any direction: the whole point of the step. + it('keeps arc neighbours one step apart and clear of the hub', () => { + for (const count of [1, 2, 3, 5, 8]) { + const points = fanOffsets('arc', count, STEP) + const hub = { x: 0, y: 0 } + + expect(points).toHaveLength(count) + + for (const p of points) { + expect(p.y).toBeLessThan(0) + expect(dist(p, hub)).toBeGreaterThanOrEqual(STEP - 1e-6) + } + + for (let i = 1; i < count; i++) { + expect(dist(points[i - 1], points[i])).toBeCloseTo(STEP, 6) + } + } + }) + + it('centres the arc over the hub', () => { + const points = fanOffsets('arc', 4, STEP) + + expect(points[0].x).toBeCloseTo(-points[3].x, 6) + expect(points[1].x).toBeCloseTo(-points[2].x, 6) + }) +}) diff --git a/apps/desktop/src/components/ui/fan-menu.tsx b/apps/desktop/src/components/ui/fan-menu.tsx new file mode 100644 index 0000000000..895b798670 --- /dev/null +++ b/apps/desktop/src/components/ui/fan-menu.tsx @@ -0,0 +1,397 @@ +import { + type CSSProperties, + memo, + type ReactNode, + useCallback, + useEffect, + useLayoutEffect, + useRef, + useState +} from 'react' +import { createPortal } from 'react-dom' + +import { Button } from '@/components/ui/button' +import { Codicon } from '@/components/ui/codicon' +import { Tip } from '@/components/ui/tooltip' +import { cn } from '@/lib/utils' + +export type FanDirection = 'horizontal' | 'vertical' | 'arc' + +export interface FanMenuItem { + id: string + icon: ReactNode + label: string + /** A toggle that is ON. Solid primary disc, so it reads from across the room. */ + active?: boolean + disabled?: boolean + onSelect: () => void +} + +export interface FanMenuProps { + /** The always-visible control. Hovering or focusing it fans `items` out. */ + hub: FanMenuItem & { className?: string } + items: readonly FanMenuItem[] + direction?: FanDirection + /** Accessible name for the fanned group. */ + label: string + /** Gap between discs, px. Discs are the hub's measured size; the step + * between them is that size plus this. */ + gap?: number + /** Where each disc's tooltip sits. Defaults to left, so tips hang off the + * side of the fan rather than covering the next disc up. */ + tipAnchor?: 'top' | 'left' | 'right' | 'bottom' + /** The hairline caret on the hub that says "there is more". */ + hint?: boolean +} + +interface Point { + x: number + y: number +} + +/** Quick with a touch of overshoot: the discs pop into place, not float. */ +const POP = 'cubic-bezier(0.2, 1.25, 0.4, 1)' +const OPEN_MS = 160 +const CLOSE_MS = 100 +const STAGGER_MS = 20 +/** Grace while the pointer crosses the gap between the hub and a disc. */ +const HIDE_DELAY_MS = 150 +/** Widest an arc will spread before it grows its radius instead. */ +const ARC_MAX_SPAN_DEG = 150 + +/** + * Disc centres relative to the hub, in px. `step` is one disc plus one gap, + * so neighbours never touch in any direction. + * + * - vertical: a column straight up, in order. + * - horizontal: reading order, split around the hub — the first half to the + * left, the rest to the right, the extra one (odd counts) on the right. + * - arc: a semicircle above, spread so adjacent discs sit one `step` apart + * along the chord; past 150° the radius grows to fit instead. + */ +export function fanOffsets(direction: FanDirection, count: number, step: number): Point[] { + const out: Point[] = [] + + if (direction === 'vertical') { + for (let i = 0; i < count; i++) { + out.push({ x: 0, y: -(i + 1) * step }) + } + + return out + } + + if (direction === 'horizontal') { + const left = Math.floor(count / 2) + + for (let i = 0; i < count; i++) { + out.push({ x: i < left ? -(left - i) * step : (i - left + 1) * step, y: 0 }) + } + + return out + } + + if (count === 1) { + return [{ x: 0, y: -step }] + } + + let radius = step * 1.15 + let delta = 2 * Math.asin(Math.min(1, step / (2 * radius))) + const maxSpan = (ARC_MAX_SPAN_DEG * Math.PI) / 180 + + if (delta * (count - 1) > maxSpan) { + delta = maxSpan / (count - 1) + radius = step / (2 * Math.sin(delta / 2)) + } + + const start = Math.PI / 2 + (delta * (count - 1)) / 2 + + for (let i = 0; i < count; i++) { + const a = start - i * delta + + out.push({ x: Math.cos(a) * radius, y: -Math.sin(a) * radius }) + } + + return out +} + +const sameRect = (a: DOMRect | null, b: DOMRect) => + a !== null && a.left === b.left && a.top === b.top && a.width === b.width && a.height === b.height + +/** + * One control that fans its siblings out on hover — a column, a row centred + * on it, or an arc. The hub stays in the flow where the consumer puts it; the + * discs portal to `body` so an `overflow-hidden` parent cannot clip them, and + * are re-anchored on scroll and resize. + * + * Hover, geometry and open/close timing live here. The consumer only + * re-renders when its own items change; pointer traffic never reaches it. + */ +export function FanMenu({ + direction = 'vertical', + gap = 4, + hint = true, + hub, + items, + label, + tipAnchor = 'left' +}: FanMenuProps) { + const anchorRef = useRef(null) + const [rect, setRect] = useState(null) + const [open, setOpen] = useState(false) + const hideTimer = useRef(undefined) + const unmountTimer = useRef(undefined) + + const clearTimers = useCallback(() => { + window.clearTimeout(hideTimer.current) + window.clearTimeout(unmountTimer.current) + hideTimer.current = undefined + }, []) + + const measure = useCallback(() => { + const el = anchorRef.current + + if (el) { + const next = el.getBoundingClientRect() + + setRect(current => (sameRect(current, next) ? current : next)) + } + }, []) + + const show = useCallback(() => { + clearTimers() + measure() + setOpen(true) + }, [clearTimers, measure]) + + const hide = useCallback(() => { + clearTimers() + setOpen(false) + unmountTimer.current = window.setTimeout(() => setRect(null), CLOSE_MS + STAGGER_MS * items.length) + }, [clearTimers, items.length]) + + const hideSoon = useCallback(() => { + window.clearTimeout(hideTimer.current) + hideTimer.current = window.setTimeout(hide, HIDE_DELAY_MS) + }, [hide]) + + const stayOpen = useCallback(() => { + window.clearTimeout(hideTimer.current) + hideTimer.current = undefined + }, []) + + useEffect(() => clearTimers, [clearTimers]) + + // The hub can move under a parked fan: its container grows, the window + // resizes, an ancestor scrolls. + const anchored = rect !== null + + useEffect(() => { + if (!anchored) { + return + } + + window.addEventListener('scroll', measure, true) + window.addEventListener('resize', measure) + + return () => { + window.removeEventListener('scroll', measure, true) + window.removeEventListener('resize', measure) + } + }, [anchored, measure]) + + // Enter/leave alone can strand the fan open. A disc that goes `disabled` + // under the pointer (a toggle turning pending right after the click) stops + // receiving pointer events, so its `pointerleave` never fires; a fast + // flick can skip the leave the same way. Watch the document while open and + // close as soon as the pointer is over neither the hub nor a disc. + useEffect(() => { + if (!open) { + return + } + + const onMove = (event: PointerEvent) => { + const target = event.target instanceof Element ? event.target : null + + const inside = + target !== null && + (anchorRef.current?.contains(target) === true || target.closest('[data-slot="fan-menu"]') !== null) + + if (inside) { + stayOpen() + } else if (hideTimer.current === undefined) { + hideSoon() + } + } + + document.addEventListener('pointermove', onMove, true) + + return () => document.removeEventListener('pointermove', onMove, true) + }, [hideSoon, open, stayOpen]) + + const leaveUnlessWithin = (event: React.FocusEvent, then: () => void) => { + if (!(event.relatedTarget instanceof Node && event.currentTarget.contains(event.relatedTarget))) { + then() + } + } + + return ( + <> + {/* The wrapper takes the hover, not the button: a disabled hub swallows + pointer events, and the fan must still open around it. */} + leaveUnlessWithin(event, hideSoon)} + onFocus={show} + onPointerEnter={show} + onPointerLeave={hideSoon} + ref={anchorRef} + > + + + + {hint ? ( + + ) : null} + + {rect + ? createPortal( + , + document.body + ) + : null} + + ) +} + +function discStyle(target: Point, index: number, entered: boolean): CSSProperties { + // Folded, a disc sits halfway to its slot rather than under the hub: one + // that mounts beneath a parked pointer picks up :hover and keeps it (a + // transform-only move never re-hit-tests), so it would pop out pre-lit. + const k = entered ? 1 : 0.5 + const delay = index * STAGGER_MS + + return { + opacity: entered ? 1 : 0, + scale: entered ? '1' : '0.5', + translate: `${(target.x * k).toFixed(2)}px ${(target.y * k).toFixed(2)}px`, + transition: entered + ? `translate ${OPEN_MS}ms ${POP} ${delay}ms, scale ${OPEN_MS}ms ${POP} ${delay}ms, opacity 80ms ease-out ${delay}ms` + : `translate ${CLOSE_MS}ms ease-in, scale ${CLOSE_MS}ms ease-in, opacity ${CLOSE_MS}ms ease-in` + } +} + +const FanCluster = memo(function FanCluster({ + direction, + items, + label, + onClose, + onPointerEnter, + onPointerLeave, + open, + rect, + step, + tipAnchor +}: { + direction: FanDirection + items: readonly FanMenuItem[] + label: string + onClose: () => void + onPointerEnter: () => void + onPointerLeave: () => void + open: boolean + rect: DOMRect + step: number + tipAnchor: NonNullable +}) { + const rootRef = useRef(null) + // Mount folded, then unfold on the next style pass so the translate + // actually transitions instead of landing in place. The layout read forces + // Chromium to resolve the folded style before the change. + const [entered, setEntered] = useState(false) + + useLayoutEffect(() => { + if (open) { + void rootRef.current?.offsetWidth + } + + setEntered(open) + }, [open]) + + const offsets = fanOffsets(direction, items.length, step) + + return ( + // The root is the hub's footprint and takes no pointer events itself, so + // the real hub underneath keeps its hover; only the discs are hittable. +
{ + if (event.key === 'Escape') { + event.stopPropagation() + onClose() + } + }} + ref={rootRef} + role="group" + style={{ height: rect.height, left: rect.left, top: rect.top, width: rect.width }} + > + {items.map((item, index) => ( + + + + ))} +
+ ) +}) From bc722dcb85c55f01f5ddbe4c85042ff74893ba9a Mon Sep 17 00:00:00 2001 From: brooklyn! Date: Wed, 16 Sep 2026 00:35:51 -0700 Subject: [PATCH 16/18] feat(desktop): fan the composer's voice toggles out of the mic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The docked composer spent three icon buttons on toggles that are set once and rarely touched. The mic is now the one button in the row; hovering it fans spoken-replies and the wake word out above it. The mic reports dictation only — the wake word carries its own on-state on its own disc, so mirroring it onto the mic read as dictation being on. The wake disc's tip names the phrase and nothing else; the pressed state says on/off. The folded HUD/narrow-tile VoiceMenu is unchanged. --- .../src/app/chat/composer/controls.test.tsx | 56 +++++---- .../src/app/chat/composer/controls.tsx | 109 +++-------------- .../src/app/chat/composer/voice-fan.tsx | 110 ++++++++++++++++++ apps/desktop/src/i18n/en.ts | 1 + apps/desktop/src/i18n/ja.ts | 1 + apps/desktop/src/i18n/ru.ts | 1 + apps/desktop/src/i18n/types.ts | 1 + apps/desktop/src/i18n/zh-hant.ts | 1 + apps/desktop/src/i18n/zh.ts | 1 + 9 files changed, 165 insertions(+), 116 deletions(-) create mode 100644 apps/desktop/src/app/chat/composer/voice-fan.tsx diff --git a/apps/desktop/src/app/chat/composer/controls.test.tsx b/apps/desktop/src/app/chat/composer/controls.test.tsx index d5694b6144..5b8ff62b24 100644 --- a/apps/desktop/src/app/chat/composer/controls.test.tsx +++ b/apps/desktop/src/app/chat/composer/controls.test.tsx @@ -61,19 +61,28 @@ afterEach(() => { $hudMode.set(false) }) -// The HUD is a Spotlight bar a few hundred pixels wide: the four voice -// controls fold into one menu there, and the way out of HUD mode joins the -// row instead of floating above the bar in a reserved strip. The docked -// composer keeps every control inline and shows no exit. +// The HUD is a Spotlight bar a few hundred pixels wide: the voice controls +// fold into one menu there, and the way out of HUD mode joins the row instead +// of floating above the bar in a reserved strip. The docked composer keeps the +// mic inline, with the other voice toggles fanned out of it on hover, and +// shows no exit. describe('HUD mode', () => { - it('keeps the voice controls inline and offers no exit in the docked composer', () => { + it('keeps the mic inline, fans the toggles on hover, and offers no exit in the docked composer', async () => { renderControls() - expect(screen.getByLabelText('Voice dictation')).toBeTruthy() - expect(screen.getByLabelText('Read replies aloud')).toBeTruthy() + const mic = screen.getByLabelText('Voice dictation') + + expect(mic).toBeTruthy() + expect(screen.queryByLabelText('Read replies aloud')).toBeNull() + + fireEvent.pointerEnter(mic.parentElement!) + + expect(await screen.findByLabelText('Read replies aloud')).toBeTruthy() + expect(screen.getByLabelText('Wake word "hey hermes"')).toBeTruthy() expect(screen.queryByLabelText('Exit HUD mode')).toBeNull() expect(screen.queryByLabelText('Reset HUD size and position')).toBeNull() - expect(screen.queryByLabelText('Voice')).toBeNull() + // No folded menu trigger — the fan's group shares the "Voice" name. + expect(screen.queryByRole('button', { name: 'Voice' })).toBeNull() }) it('folds them into one menu and offers the way out in the HUD', () => { @@ -162,38 +171,37 @@ describe('wake-word ear visibility', () => { resetWakeWordState() }) - it('stays mounted during a busy agent turn', () => { + // The ear lives in the mic's fan now: hover the mic to reach it. + const findEar = async () => { + fireEvent.pointerEnter(screen.getByLabelText('Voice dictation').parentElement!) + + return screen.findByLabelText('Wake word "hey hermes"') + } + + it('stays reachable during a busy agent turn', async () => { applyWakeStatus({ available: true, enabled: true, listening: true, phrase: 'hey hermes' }) renderControls({ busy: true, busyAction: 'stop' }) - expect(screen.getByLabelText('Wake word: "hey hermes" — listening')).toBeTruthy() + expect((await findEar()).getAttribute('aria-pressed')).toBe('true') }) - it('stays mounted (enabled in config) even when a start was refused', () => { + it('stays reachable (enabled in config) even when a start was refused', async () => { applyWakeStatus({ available: true, enabled: true, listening: false, phrase: 'hey hermes' }) // Transient refusal marks available false but enabled keeps it mounted. applyWakeStartResult({ hint: 'mic busy', reason: 'unavailable', started: false }) renderControls() - expect(screen.getByLabelText('Wake word: "hey hermes" — off')).toBeTruthy() + expect((await findEar()).getAttribute('aria-pressed')).toBe('false') }) - it('stays visible (never hides) even when unavailable and not enabled', () => { - applyWakeStatus({ available: false, enabled: false, listening: false, phrase: 'hey hermes' }) - renderControls() - - // The ear ALWAYS shows so the user can click to enable; a failed start - // surfaces its reason in the tooltip rather than hiding the control. - expect(screen.getByLabelText('Wake word: "hey hermes" — off')).toBeTruthy() - }) - - it('surfaces the backend refusal reason in the tooltip, still visible', () => { + it('stays reachable (never hides) even when unavailable and not enabled', async () => { applyWakeStatus({ available: false, enabled: false, listening: false, phrase: 'hey hermes' }) applyWakeStartResult({ hint: 'run `hermes tools` (Voice section)', reason: 'unavailable', started: false }) renderControls() - const ear = screen.getByLabelText('Wake word: "hey hermes" — off') - expect(ear).toBeTruthy() + // The ear ALWAYS shows so the user can click to enable; a refused start + // never hides the control. + expect(((await findEar()) as HTMLButtonElement).disabled).toBe(false) }) it('shows a disabled paused ear inside the voice-conversation pill', () => { diff --git a/apps/desktop/src/app/chat/composer/controls.tsx b/apps/desktop/src/app/chat/composer/controls.tsx index 62e1a59234..09ecd91e50 100644 --- a/apps/desktop/src/app/chat/composer/controls.tsx +++ b/apps/desktop/src/app/chat/composer/controls.tsx @@ -5,7 +5,7 @@ import { Codicon } from '@/components/ui/codicon' import { Tip, TipKeybindLabel } from '@/components/ui/tooltip' import { useI18n } from '@/i18n' import { triggerHaptic } from '@/lib/haptics' -import { Ear, EarOff, iconSize, Layers3, Loader2, Square, Volume2, VolumeX } from '@/lib/icons' +import { Ear, EarOff, iconSize, Layers3, Loader2, Square } from '@/lib/icons' import { cn } from '@/lib/utils' import { $hudMode, closeHud, resetHudLayout } from '@/store/hud' import { $wakeWord, toggleWakeWord } from '@/store/wake-word' @@ -16,6 +16,7 @@ import { ModelPill } from './model-pill' import { ReasoningPill } from './reasoning-pill' import { StartVoiceButton } from './start-voice-button' import type { ChatBarState, VoiceStatus } from './types' +import { VoiceFan } from './voice-fan' import { VoiceMenu } from './voice-menu' // Re-exported: `context-menu.tsx` and other row neighbours have always reached @@ -100,11 +101,15 @@ export function ComposerControls({ voiceStatus={voiceStatus} /> ) : ( - <> - - - - + // One mic in the row; hovering it fans the other voice toggles out of it. + ) return ( @@ -121,7 +126,7 @@ export function ComposerControls({ )} {showQueueButton ? ( - }> + } side="left"> - + - - ) -} - // "Hey Hermes" wake-word toggle. ALWAYS rendered — the ear never hides. A // user must always be able to click it to turn passive listening on; if the // backend can't start (missing STT/TTS, deps still installing, no mic @@ -370,7 +347,7 @@ function WakeWordButton({ disabled, pausedForVoice = false }: { disabled: boolea const tooltip = !pausedForVoice && wake.notice ? `${label} — ${wake.notice}` : label return ( - + - - ) -} diff --git a/apps/desktop/src/app/chat/composer/voice-fan.tsx b/apps/desktop/src/app/chat/composer/voice-fan.tsx new file mode 100644 index 0000000000..88ee3a8278 --- /dev/null +++ b/apps/desktop/src/app/chat/composer/voice-fan.tsx @@ -0,0 +1,110 @@ +import { useStore } from '@nanostores/react' +import { useCallback, useMemo } from 'react' + +import { Codicon } from '@/components/ui/codicon' +import { FanMenu, type FanMenuItem } from '@/components/ui/fan-menu' +import { useI18n } from '@/i18n' +import { triggerHaptic } from '@/lib/haptics' +import { Ear, EarOff, iconSize, Loader2, Square, Volume2, VolumeX } from '@/lib/icons' +import { cn } from '@/lib/utils' +import { $wakeWord, toggleWakeWord } from '@/store/wake-word' + +import { ACTIVE_ICON_BTN, GHOST_ICON_BTN } from './control-classes' +import type { ChatBarState, VoiceStatus } from './types' + +export interface VoiceFanProps { + autoSpeak: boolean + disabled: boolean + state: ChatBarState + voiceStatus: VoiceStatus + onDictate: () => void + onToggleAutoSpeak: () => void +} + +/** + * The voice toggles behind one hub. The mic is the button in the row; hovering + * it fans the other two — spoken replies and the wake word — out of it. + * Starting a conversation stays on the primary button beside it. + * + * The hub is the mic and reports dictation only (recording, transcribing); + * each disc carries its own on-state. + * + * Items are memoized on the handful of state bits they read, so the fan only + * re-renders when a toggle actually flips — not on every composer keystroke. + */ +export function VoiceFan({ autoSpeak, disabled, state, voiceStatus, onDictate, onToggleAutoSpeak }: VoiceFanProps) { + const { t } = useI18n() + const c = t.composer + const wake = useStore($wakeWord) + + const phrase = wake.phrase || 'hey hermes' + const dictating = state.voice.active || voiceStatus !== 'idle' + const wakeListening = wake.listening + const wakePending = wake.pending + + const dictationLabel = + voiceStatus === 'recording' + ? c.stopDictation + : voiceStatus === 'transcribing' + ? c.transcribingDictation + : c.voiceDictation + + const hubLabel = dictating ? dictationLabel : c.voiceDictation + + const dictate = useCallback(() => { + triggerHaptic(dictating ? 'close' : 'open') + onDictate() + }, [dictating, onDictate]) + + // The hub is the mic and only dictation lights it. The wake word has its own + // disc with its own on-state; mirroring it here read as dictation being on. + const hub = useMemo( + () => ({ + id: 'dictate', + active: dictating, + className: cn(GHOST_ICON_BTN, 'rounded-full p-0', dictating && ACTIVE_ICON_BTN), + disabled: disabled || !state.voice.enabled || voiceStatus === 'transcribing', + icon: + voiceStatus === 'recording' ? ( + + ) : voiceStatus === 'transcribing' ? ( + + ) : ( + + ), + label: hubLabel, + onSelect: dictate + }), + [dictate, dictating, disabled, hubLabel, state.voice.enabled, voiceStatus] + ) + + const items = useMemo( + () => [ + { + id: 'speak', + active: autoSpeak, + disabled, + icon: autoSpeak ? : , + label: autoSpeak ? c.stopSpeakingReplies : c.speakReplies, + onSelect: () => { + triggerHaptic(autoSpeak ? 'close' : 'open') + onToggleAutoSpeak() + } + }, + { + id: 'wake', + active: wakeListening, + disabled: disabled || wakePending, + icon: wakeListening ? : , + label: c.wakeWord(phrase), + onSelect: () => { + triggerHaptic(wakeListening ? 'close' : 'open') + void toggleWakeWord() + } + } + ], + [autoSpeak, c, disabled, onToggleAutoSpeak, phrase, wakeListening, wakePending] + ) + + return +} diff --git a/apps/desktop/src/i18n/en.ts b/apps/desktop/src/i18n/en.ts index 0ce996a314..1d2ce02324 100644 --- a/apps/desktop/src/i18n/en.ts +++ b/apps/desktop/src/i18n/en.ts @@ -2908,6 +2908,7 @@ export const en: Translations = { voiceDictation: 'Voice dictation', speakReplies: 'Read replies aloud', stopSpeakingReplies: 'Stop reading replies aloud', + wakeWord: phrase => `Wake word "${phrase}"`, wakeWordListening: phrase => `Wake word: "${phrase}" — listening`, wakeWordOff: phrase => `Wake word: "${phrase}" — off`, wakeWordPausedVoice: phrase => `Wake word: "${phrase}" — paused during voice chat`, diff --git a/apps/desktop/src/i18n/ja.ts b/apps/desktop/src/i18n/ja.ts index 6993ff90d5..3d1b247308 100644 --- a/apps/desktop/src/i18n/ja.ts +++ b/apps/desktop/src/i18n/ja.ts @@ -2401,6 +2401,7 @@ export const ja = defineLocale({ speakReplies: '返信を読み上げる', stopSpeakingReplies: '返信の読み上げを停止', wakeWordListening: phrase => `ウェイクワード:「${phrase}」— 待機中`, + wakeWord: phrase => `ウェイクワード「${phrase}」`, wakeWordOff: phrase => `ウェイクワード:「${phrase}」— オフ`, wakeWordPausedVoice: phrase => `ウェイクワード:「${phrase}」— 音声チャット中は一時停止`, lookupLoading: '検索中…', diff --git a/apps/desktop/src/i18n/ru.ts b/apps/desktop/src/i18n/ru.ts index 803ad1e69e..df21d80553 100644 --- a/apps/desktop/src/i18n/ru.ts +++ b/apps/desktop/src/i18n/ru.ts @@ -2674,6 +2674,7 @@ export const ru = defineLocale({ voiceDictation: 'Голосовая диктовка', speakReplies: 'Зачитывать ответы вслух', stopSpeakingReplies: 'Перестать зачитывать ответы вслух', + wakeWord: phrase => `Слово-пробуждение «${phrase}»`, wakeWordListening: phrase => `Слово-пробуждение: «${phrase}» — слушает`, wakeWordOff: phrase => `Слово-пробуждение: «${phrase}» — выключено`, wakeWordPausedVoice: phrase => `Слово-пробуждение: «${phrase}» — приостановлено во время голосового чата`, diff --git a/apps/desktop/src/i18n/types.ts b/apps/desktop/src/i18n/types.ts index 62fdf475e0..536d29b029 100644 --- a/apps/desktop/src/i18n/types.ts +++ b/apps/desktop/src/i18n/types.ts @@ -2513,6 +2513,7 @@ export interface Translations { voiceDictation: string speakReplies: string stopSpeakingReplies: string + wakeWord: (phrase: string) => string wakeWordListening: (phrase: string) => string wakeWordOff: (phrase: string) => string wakeWordPausedVoice: (phrase: string) => string diff --git a/apps/desktop/src/i18n/zh-hant.ts b/apps/desktop/src/i18n/zh-hant.ts index 6f5e79005f..c434702518 100644 --- a/apps/desktop/src/i18n/zh-hant.ts +++ b/apps/desktop/src/i18n/zh-hant.ts @@ -2383,6 +2383,7 @@ export const zhHant = defineLocale({ speakReplies: '朗讀回覆', stopSpeakingReplies: '停止朗讀回覆', wakeWordListening: phrase => `喚醒詞:「${phrase}」— 正在聆聽`, + wakeWord: phrase => `喚醒詞「${phrase}」`, wakeWordOff: phrase => `喚醒詞:「${phrase}」— 已關閉`, wakeWordPausedVoice: phrase => `喚醒詞:「${phrase}」— 語音對話期間暫停`, lookupLoading: '查詢中…', diff --git a/apps/desktop/src/i18n/zh.ts b/apps/desktop/src/i18n/zh.ts index b31f823cc2..1ab0cf0b68 100644 --- a/apps/desktop/src/i18n/zh.ts +++ b/apps/desktop/src/i18n/zh.ts @@ -3051,6 +3051,7 @@ export const zh = defineLocale({ speakReplies: '朗读回复', stopSpeakingReplies: '停止朗读回复', wakeWordListening: phrase => `唤醒词:"${phrase}" — 正在监听`, + wakeWord: phrase => `唤醒词"${phrase}"`, wakeWordOff: phrase => `唤醒词:"${phrase}" — 已关闭`, wakeWordPausedVoice: phrase => `唤醒词:"${phrase}" — 语音对话期间暂停`, lookupLoading: '查找中…', From 1331053fcf0398ce04698a75a7e4c82b74a2fc1a Mon Sep 17 00:00:00 2001 From: brooklyn! Date: Wed, 16 Sep 2026 00:35:51 -0700 Subject: [PATCH 17/18] style(desktop): anchor composer icon tooltips beside the control Tips on the composer's row controls open to the left so they never cover the fanned discs or the input; the model and reasoning pills keep theirs centred above. A left-anchored Tip rags its wrapped lines toward the trigger, in the primitive rather than per call site. --- apps/desktop/src/app/chat/composer/context-menu.tsx | 2 +- .../src/app/chat/composer/start-voice-button.tsx | 4 ++-- apps/desktop/src/app/chat/composer/voice-menu.tsx | 2 +- apps/desktop/src/components/ui/tooltip.tsx | 10 +++++++++- 4 files changed, 13 insertions(+), 5 deletions(-) diff --git a/apps/desktop/src/app/chat/composer/context-menu.tsx b/apps/desktop/src/app/chat/composer/context-menu.tsx index b63149e8b1..b0e41c1c67 100644 --- a/apps/desktop/src/app/chat/composer/context-menu.tsx +++ b/apps/desktop/src/app/chat/composer/context-menu.tsx @@ -47,7 +47,7 @@ export function ContextMenu({ return ( <> - +