From 2bc82bb504a7a384624176cbbe1c63c2007ed48e Mon Sep 17 00:00:00 2001 From: emozilla Date: Mon, 4 May 2026 15:31:15 -0400 Subject: [PATCH 01/21] clarify placeholder telegram credential in tests --- tests/hermes_cli/test_env_loader.py | 4 ++-- tests/hermes_cli/test_env_sanitize_on_load.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/hermes_cli/test_env_loader.py b/tests/hermes_cli/test_env_loader.py index f94649a634..f309dfd4c6 100644 --- a/tests/hermes_cli/test_env_loader.py +++ b/tests/hermes_cli/test_env_loader.py @@ -37,7 +37,7 @@ def test_project_env_is_sanitized_before_loading(tmp_path, monkeypatch): home = tmp_path / "hermes" project_env = tmp_path / ".env" project_env.write_text( - "TELEGRAM_BOT_TOKEN=8356550917:AAGGEkzg06Hrc3Hjb3Sa1jkGVDOdU_lYy2Q" + "TELEGRAM_BOT_TOKEN=0123456789:test" "ANTHROPIC_API_KEY=sk-ant-test123\n", encoding="utf-8", ) @@ -48,7 +48,7 @@ def test_project_env_is_sanitized_before_loading(tmp_path, monkeypatch): loaded = load_hermes_dotenv(hermes_home=home, project_env=project_env) assert loaded == [project_env] - assert os.getenv("TELEGRAM_BOT_TOKEN") == "8356550917:AAGGEkzg06Hrc3Hjb3Sa1jkGVDOdU_lYy2Q" + assert os.getenv("TELEGRAM_BOT_TOKEN") == "0123456789:test" assert os.getenv("ANTHROPIC_API_KEY") == "sk-ant-test123" diff --git a/tests/hermes_cli/test_env_sanitize_on_load.py b/tests/hermes_cli/test_env_sanitize_on_load.py index 6ac7c2cef3..f23eadd2a5 100644 --- a/tests/hermes_cli/test_env_sanitize_on_load.py +++ b/tests/hermes_cli/test_env_sanitize_on_load.py @@ -14,7 +14,7 @@ def test_load_env_sanitizes_concatenated_lines(): """ from hermes_cli.config import load_env - token = "8356550917:AAGGEkzg06Hrc3Hjb3Sa1jkGVDOdU_lYy2Q" + token = "0123456789:test" # Simulate concatenated line: TOKEN=xxx followed immediately by another key corrupted = f"TELEGRAM_BOT_TOKEN={token}ANTHROPIC_API_KEY=sk-ant-test123\n" @@ -67,7 +67,7 @@ def test_env_loader_sanitizes_before_dotenv(): """Verify env_loader._sanitize_env_file_if_needed fixes corrupted files.""" from hermes_cli.env_loader import _sanitize_env_file_if_needed - token = "8356550917:AAGGEkzg06Hrc3Hjb3Sa1jkGVDOdU_lYy2Q" + token = "0123456789:test" corrupted = f"TELEGRAM_BOT_TOKEN={token}ANTHROPIC_API_KEY=sk-ant-test\n" with tempfile.NamedTemporaryFile( From 429b8eceb4e053f8783440396c18e30d41509712 Mon Sep 17 00:00:00 2001 From: nftpoetrist Date: Tue, 5 May 2026 00:45:01 +0300 Subject: [PATCH 02/21] fix(cli): guard c-S-c key binding with try/except to prevent startup crash (#19895) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #19884 added @kb.add('c-S-c') unconditionally. prompt_toolkit raises ValueError("Invalid key: c-S-c") during HermesCLI.__init__ on platforms where this key spec is not recognised — the process exits before reaching the prompt loop. Reported on macOS (#19894) and Linux (#19896) immediately after #19884 landed. Fix: wrap the registration in try/except ValueError so that startup continues cleanly on any platform/version that rejects the spec. Where the spec is accepted the binding is registered normally as a no-op, allowing the terminal to handle Ctrl+Shift+C natively as before. Fixes #19894 Fixes #19896 --- cli.py | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/cli.py b/cli.py index 96ecb8ecfd..2c5df6fbc4 100644 --- a/cli.py +++ b/cli.py @@ -10484,20 +10484,19 @@ class HermesCLI: self._should_exit = True event.app.exit() - @kb.add('c-S-c') # Ctrl+Shift+C - def handle_ctrl_shift_c(event): - """Copy text to clipboard (terminal-native). + try: + @kb.add('c-S-c') # Ctrl+Shift+C — no-op, let terminal handle native copy + def handle_ctrl_shift_c(event): + """No-op: let the terminal handle Ctrl+Shift+C natively. - This is a no-op at the application level. Terminal emulators - handle the actual copy operation when Ctrl+Shift+C is pressed. - This binding prevents Hermes from intercepting the keystroke - as an interrupt signal. - - On macOS the standard copy shortcut is Cmd+C (no Hermes binding - needed). On Linux/Windows Ctrl+Shift+C is the conventional - terminal copy shortcut. - """ - return # No-op — let the terminal perform native copy + Wrapped in try/except because prompt_toolkit raises ValueError + ("Invalid key: c-S-c") on platforms where this key spec is not + recognised. The binding is best-effort; if registration fails, + startup continues normally. + """ + return + except ValueError: + pass # prompt_toolkit on this platform/version doesn't support c-S-c @kb.add('c-q') # Ctrl+Q def handle_ctrl_q(event): From 56a78e74b26a10bd87578e019f9ef84e76a90b5d Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Mon, 4 May 2026 14:48:19 -0700 Subject: [PATCH 03/21] =?UTF-8?q?feat(kanban-dashboard):=20sharper=20home-?= =?UTF-8?q?channel=20toggle=20contrast,=20drop=20=E2=86=92=20running=20act?= =?UTF-8?q?ion=20(#19916)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up polish to the kanban dashboard from #19864 and #19705. **Home-channel toggle contrast.** The `.hermes-kanban-home-sub--on` class previously used `color-mix(var(--color-ring) 14%, transparent)` which was nearly invisible on both the default teal and NERV themes — the on/off distinction relied almost entirely on the ✓ prefix glyph. Bump to 32% fill + full-opacity ring border + inner ring shadow + font-weight 600. Still theme-scoped (no hardcoded colors), but reads at a glance on both tested themes. **Drop the → running status action.** Since #19705, `PATCH /tasks/:id` rejects `status=running` with HTTP 400 — only the dispatcher's `claim_task` path legitimately enters that state (so the run row, claim lock, and worker PID are created atomically). The UI button was still present and produced a 400 on click, which is a confusing dead affordance. Remove it from `StatusActions`; add a comment pointing to #19535 so future editors know why it's missing. Live-tested on the default Hermes Teal theme. 53/53 kanban dashboard plugin tests still pass. --- plugins/kanban/dashboard/dist/index.js | 5 ++++- plugins/kanban/dashboard/dist/style.css | 12 ++++++++---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/plugins/kanban/dashboard/dist/index.js b/plugins/kanban/dashboard/dist/index.js index d60bc19289..ee1a25f065 100644 --- a/plugins/kanban/dashboard/dist/index.js +++ b/plugins/kanban/dashboard/dist/index.js @@ -1996,7 +1996,10 @@ return h("div", { className: "hermes-kanban-actions" }, b("→ triage", { status: "triage" }, t.status !== "triage"), b("→ ready", { status: "ready" }, t.status !== "ready"), - b("→ running", { status: "running" }, t.status !== "running"), + // No direct → running button: /tasks/:id PATCH rejects status=running + // with 400 (issue #19535). Tasks enter running only through the + // dispatcher's claim_task path, which atomically creates the run row, + // claim lock, and worker process metadata. b("Block", { status: "blocked" }, t.status === "running" || t.status === "ready", DESTRUCTIVE_TRANSITIONS.blocked), diff --git a/plugins/kanban/dashboard/dist/style.css b/plugins/kanban/dashboard/dist/style.css index 34fc714d11..9de372cbe2 100644 --- a/plugins/kanban/dashboard/dist/style.css +++ b/plugins/kanban/dashboard/dist/style.css @@ -364,11 +364,15 @@ letter-spacing: 0.02em; } .hermes-kanban-home-sub--on { - /* Subtly indicate the subscribed state without a hard color change so - * dashboard themes stay coherent. Border + tinted background. */ - border-color: color-mix(in srgb, var(--color-ring) 55%, var(--color-border)); - background: color-mix(in srgb, var(--color-ring) 14%, transparent); + /* Subscribed toggle — use a strong ring-colored accent so the on/off + * distinction reads at a glance, not just from the ✓ prefix. Border + + * filled background + bolder weight keep the state obvious across + * themes (tested on default teal and NERV orange). */ + border-color: var(--color-ring); + background: color-mix(in srgb, var(--color-ring) 32%, transparent); color: var(--color-foreground); + font-weight: 600; + box-shadow: inset 0 0 0 1px color-mix(in srgb, var(--color-ring) 40%, transparent); } .hermes-kanban-section { From b8fb9270c4672e81e710c242e96ad1eaf60d18e0 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Mon, 4 May 2026 14:49:38 -0700 Subject: [PATCH 04/21] refactor(cli): drop dead c-S-c key binding (follow-up to #19895) (#19919) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #19884 added a prompt_toolkit key binding for Ctrl+Shift+C to "prevent Hermes from intercepting the keystroke as an interrupt signal." #19895 then wrapped the binding in try/except after discovering it crashed startup with ValueError on every platform. Both PRs were based on a misreading of how terminal key events propagate: 1. Terminal emulators (GNOME Terminal, iTerm2, kitty, Windows Terminal, etc.) intercept Ctrl+Shift+C before the keystroke reaches the application's stdin. prompt_toolkit never sees it. The binding could never have intercepted anything. 2. prompt_toolkit's key spec parser doesn't recognise 'c-S-c' on any platform — the Shift modifier is meaningless on control-sequence keys. Verified: every prompt_toolkit version raises 'Invalid key: c-S-c' at registration time. The handler is dead code. Delete it and leave a comment explaining why no binding is needed here. Ctrl+Q alias (#19884's other addition) stays — that's a real prompt_toolkit key and a legitimate interrupt shortcut. Verified the CLI starts cleanly — key binding phase no longer raises and the subsequent chat flow reaches the provider setup check without error. --- cli.py | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/cli.py b/cli.py index 2c5df6fbc4..eefd14fe47 100644 --- a/cli.py +++ b/cli.py @@ -10484,19 +10484,14 @@ class HermesCLI: self._should_exit = True event.app.exit() - try: - @kb.add('c-S-c') # Ctrl+Shift+C — no-op, let terminal handle native copy - def handle_ctrl_shift_c(event): - """No-op: let the terminal handle Ctrl+Shift+C natively. - - Wrapped in try/except because prompt_toolkit raises ValueError - ("Invalid key: c-S-c") on platforms where this key spec is not - recognised. The binding is best-effort; if registration fails, - startup continues normally. - """ - return - except ValueError: - pass # prompt_toolkit on this platform/version doesn't support c-S-c + # Ctrl+Shift+C: no binding needed. Terminal emulators (GNOME Terminal, + # iTerm2, kitty, Windows Terminal, etc.) intercept Ctrl+Shift+C before + # the keystroke reaches the application's stdin — prompt_toolkit never + # sees it, and prompt_toolkit's key spec parser doesn't even recognise + # 'c-S-c' anyway (the Shift modifier is meaningless on control-sequence + # keys). #19884 added a handler for this; #19895 patched the resulting + # startup crash with try/except. Both were based on a misreading of how + # terminal key events propagate. Deleting the dead handler outright. @kb.add('c-q') # Ctrl+Q def handle_ctrl_q(event): From 9644b8ae67a84e4b77f3f79f20fa87ddc44f8559 Mon Sep 17 00:00:00 2001 From: briandevans <252620095+briandevans@users.noreply.github.com> Date: Wed, 29 Apr 2026 10:16:22 -0700 Subject: [PATCH 05/21] fix(local): recover when persistent_shell cwd is deleted (#17558) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a tool call deletes its own working directory (`cd /tmp/foo && rm -rf /tmp/foo`), the next `subprocess.Popen(args, cwd=self.cwd)` raised `FileNotFoundError: [Errno 2]` before bash even started — every subsequent terminal/file-tool call hit the same wedge until the gateway restarted. Fix in `LocalEnvironment._run_bash`: before handing `self.cwd` to Popen, resolve a safe alternative when the path is gone (walk up to the nearest existing ancestor, falling back to `tempfile.gettempdir()` only as a last resort). Log a warning so the recovery is visible — not silent — and update `self.cwd` so the next call doesn't repeat the message. Defense in depth in `LocalEnvironment._update_cwd`: only adopt the new cwd when it still exists as a directory. `pwd -P` from a deleted cwd can leave a stale value in the marker file; refusing to store a missing path keeps `self.cwd` valid by construction. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/tools/test_local_env_cwd_recovery.py | 151 +++++++++++++++++++++ tools/environments/local.py | 50 ++++++- 2 files changed, 199 insertions(+), 2 deletions(-) create mode 100644 tests/tools/test_local_env_cwd_recovery.py diff --git a/tests/tools/test_local_env_cwd_recovery.py b/tests/tools/test_local_env_cwd_recovery.py new file mode 100644 index 0000000000..96e61fa1cc --- /dev/null +++ b/tests/tools/test_local_env_cwd_recovery.py @@ -0,0 +1,151 @@ +"""Tests for LocalEnvironment recovery when ``self.cwd`` is deleted. + +When a tool call inside the persistent terminal session ``rm -rf``'s its own +working directory, the next ``subprocess.Popen(..., cwd=self.cwd)`` would +otherwise raise ``FileNotFoundError`` before bash starts, wedging every +subsequent terminal/file-tool call until the gateway restarts. + +Regression coverage for https://github.com/NousResearch/hermes-agent/issues/17558. +""" + +import os +import shutil +import tempfile +import threading +from unittest.mock import MagicMock, patch + +from tools.environments.local import ( + LocalEnvironment, + _resolve_safe_cwd, +) + + +class TestResolveSafeCwd: + """Pure-function unit tests for the recovery helper.""" + + def test_returns_cwd_when_directory_exists(self, tmp_path): + path = str(tmp_path) + assert _resolve_safe_cwd(path) == path + + def test_walks_up_to_first_existing_ancestor(self, tmp_path): + nested = tmp_path / "child" / "grandchild" + nested.mkdir(parents=True) + deleted = str(nested) + shutil.rmtree(tmp_path / "child") + + # The deepest existing ancestor on the path is tmp_path itself. + assert _resolve_safe_cwd(deleted) == str(tmp_path) + + def test_falls_back_when_path_is_empty(self): + assert _resolve_safe_cwd("") == tempfile.gettempdir() + + def test_returns_tempdir_when_nothing_on_path_exists(self, monkeypatch): + monkeypatch.setattr(os.path, "isdir", lambda p: False) + assert _resolve_safe_cwd("/no/such/dir") == tempfile.gettempdir() + + +def _fake_interrupt(): + return threading.Event() + + +def _make_fake_popen(captured: dict): + def fake_popen(cmd, **kwargs): + captured["cwd"] = kwargs.get("cwd") + captured["env"] = kwargs.get("env", {}) + proc = MagicMock() + proc.poll.return_value = 0 + proc.returncode = 0 + proc.stdout = MagicMock( + __iter__=lambda s: iter([]), + __next__=lambda s: (_ for _ in ()).throw(StopIteration), + ) + proc.stdin = MagicMock() + return proc + return fake_popen + + +class TestRunBashCwdRecovery: + """End-to-end recovery: deleted ``self.cwd`` must not crash Popen.""" + + def test_recovers_when_cwd_deleted_after_init(self, tmp_path, caplog): + """Reproduces the wedge from #17558: cwd was valid when the + snapshot was taken, but a subsequent command deleted it before the + next ``Popen``.""" + wedged = tmp_path / "wedge-repro" + wedged.mkdir() + + with patch.object(LocalEnvironment, "init_session", autospec=True, return_value=None): + env = LocalEnvironment(cwd=str(wedged), timeout=10) + + # The previous tool call deleted the working directory. + shutil.rmtree(wedged) + assert env.cwd == str(wedged) and not os.path.isdir(env.cwd) + + captured = {} + with patch("tools.environments.local._find_bash", return_value="/bin/bash"), \ + patch("subprocess.Popen", side_effect=_make_fake_popen(captured)), \ + patch("tools.terminal_tool._interrupt_event", _fake_interrupt()), \ + caplog.at_level("WARNING", logger="tools.environments.local"): + env.execute("echo hello") + + # Popen must have been handed a real, existing directory. + assert captured["cwd"] == str(tmp_path) + assert os.path.isdir(captured["cwd"]) + + # ``self.cwd`` is updated so the next call doesn't re-warn. + assert env.cwd == str(tmp_path) + + # The warning surfaces the wedge so it isn't silently masked. + assert any("missing on disk" in rec.message for rec in caplog.records) + + def test_no_warning_when_cwd_still_exists(self, tmp_path, caplog): + with patch.object(LocalEnvironment, "init_session", autospec=True, return_value=None): + env = LocalEnvironment(cwd=str(tmp_path), timeout=10) + + captured = {} + with patch("tools.environments.local._find_bash", return_value="/bin/bash"), \ + patch("subprocess.Popen", side_effect=_make_fake_popen(captured)), \ + patch("tools.terminal_tool._interrupt_event", _fake_interrupt()), \ + caplog.at_level("WARNING", logger="tools.environments.local"): + env.execute("echo hello") + + assert captured["cwd"] == str(tmp_path) + assert env.cwd == str(tmp_path) + assert not any("missing on disk" in rec.message for rec in caplog.records) + + +class TestUpdateCwdRejectsMissingPaths: + """``_update_cwd`` must not propagate a deleted path back into ``self.cwd``.""" + + def test_skips_assignment_when_marker_path_missing(self, tmp_path): + original = tmp_path / "starting" + original.mkdir() + + with patch.object(LocalEnvironment, "init_session", autospec=True, return_value=None): + env = LocalEnvironment(cwd=str(original), timeout=10) + + # Simulate the stale-marker case: the prior command's ``pwd -P`` left + # a path in the cwd file, but that path has since been deleted. + deleted = tmp_path / "wedge-repro" + with open(env._cwd_file, "w") as f: + f.write(str(deleted)) + + env._update_cwd({"output": "", "returncode": 0}) + + assert env.cwd == str(original) + + def test_accepts_assignment_when_marker_path_exists(self, tmp_path): + original = tmp_path / "starting" + original.mkdir() + new_dir = tmp_path / "next" + new_dir.mkdir() + + with patch.object(LocalEnvironment, "init_session", autospec=True, return_value=None): + env = LocalEnvironment(cwd=str(original), timeout=10) + + with open(env._cwd_file, "w") as f: + f.write(str(new_dir)) + + env._update_cwd({"output": "", "returncode": 0}) + + assert env.cwd == str(new_dir) diff --git a/tools/environments/local.py b/tools/environments/local.py index 3200e63e60..99a7d9d12c 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -1,5 +1,6 @@ """Local execution environment — spawn-per-call with session snapshot.""" +import logging import os import platform import shutil @@ -12,6 +13,30 @@ from tools.environments.base import BaseEnvironment, _pipe_stdin _IS_WINDOWS = platform.system() == "Windows" +logger = logging.getLogger(__name__) + + +def _resolve_safe_cwd(cwd: str) -> str: + """Return ``cwd`` if it exists as a directory, else the nearest existing + ancestor. Falls back to ``tempfile.gettempdir()`` only if walking up the + path can't find any existing directory (effectively never on a healthy + filesystem, but cheap belt-and-braces). + + Used by ``_run_bash`` to recover when the configured cwd is gone — most + commonly because a previous tool call deleted its own working directory + (issue #17558). Without this guard, ``subprocess.Popen(..., cwd=...)`` + raises ``FileNotFoundError`` before bash starts, wedging every subsequent + terminal call until the gateway restarts. + """ + if cwd and os.path.isdir(cwd): + return cwd + parent = os.path.dirname(cwd) if cwd else "" + while parent and parent != os.path.dirname(parent): + if os.path.isdir(parent): + return parent + parent = os.path.dirname(parent) + return tempfile.gettempdir() + # Hermes-internal env vars that should NOT leak into terminal subprocesses. _HERMES_PROVIDER_ENV_FORCE_PREFIX = "_HERMES_FORCE_" @@ -358,6 +383,21 @@ class LocalEnvironment(BaseEnvironment): args = [bash, "-l", "-c", cmd_string] if login else [bash, "-c", cmd_string] run_env = _make_run_env(self.env) + # Recover when the cwd has been deleted out from under us — usually by + # a previous tool call that ran ``rm -rf`` on its own working dir + # (issue #17558). Popen would otherwise raise FileNotFoundError on + # the cwd before bash starts, wedging every subsequent call until the + # gateway restarts. + safe_cwd = _resolve_safe_cwd(self.cwd) + if safe_cwd != self.cwd: + logger.warning( + "LocalEnvironment cwd %r is missing on disk; " + "falling back to %r so terminal commands keep working.", + self.cwd, + safe_cwd, + ) + self.cwd = safe_cwd + proc = subprocess.Popen( args, text=True, @@ -452,11 +492,17 @@ class LocalEnvironment(BaseEnvironment): pass def _update_cwd(self, result: dict): - """Read CWD from temp file (local-only, no round-trip needed).""" + """Read CWD from temp file (local-only, no round-trip needed). + + Skip the assignment when the path no longer exists as a directory — + ``pwd -P`` on a deleted cwd can leave a stale value in the marker + file, and propagating it would re-wedge the next ``Popen``. The + ``_run_bash`` recovery path will resolve a safe fallback if needed. + """ try: with open(self._cwd_file) as f: cwd_path = f.read().strip() - if cwd_path: + if cwd_path and os.path.isdir(cwd_path): self.cwd = cwd_path except (OSError, FileNotFoundError): pass From 9fa3a093f29e142c4a5d97a62bc96d03175854fa Mon Sep 17 00:00:00 2001 From: briandevans <252620095+briandevans@users.noreply.github.com> Date: Wed, 29 Apr 2026 10:24:51 -0700 Subject: [PATCH 06/21] fix(local): test root as ancestor candidate; use real pipe for fake stdout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Copilot review on PR #17569: 1. _resolve_safe_cwd never tested the filesystem root because the loop exited when `os.path.dirname(parent) == parent`, which is true once `parent == '/'`. Restructure so the root is checked before the self-equal exit. Adds `test_returns_root_when_only_root_exists` — regression-guarded by reverting the loop and watching it fail. 2. The fake `Popen.stdout` was a `MagicMock`; `BaseEnvironment._wait_for_process` calls `proc.stdout.fileno()` then `select.select`/`os.read` against it, which raised `TypeError: fileno() returned a non-integer` (visible as a thread exception in test output) and could in theory read from an unrelated real fd. Hand `fake_popen` a real `os.pipe()` with the write end pre-closed so the drain loop sees EOF immediately. Helper records each fd so the test cleans up after itself. Co-Authored-By: Claude Opus 4.7 (1M context) --- tests/tools/test_local_env_cwd_recovery.py | 66 +++++++++++++++++----- tools/environments/local.py | 9 ++- 2 files changed, 58 insertions(+), 17 deletions(-) diff --git a/tests/tools/test_local_env_cwd_recovery.py b/tests/tools/test_local_env_cwd_recovery.py index 96e61fa1cc..59aa8f1067 100644 --- a/tests/tools/test_local_env_cwd_recovery.py +++ b/tests/tools/test_local_env_cwd_recovery.py @@ -43,27 +43,55 @@ class TestResolveSafeCwd: monkeypatch.setattr(os.path, "isdir", lambda p: False) assert _resolve_safe_cwd("/no/such/dir") == tempfile.gettempdir() + def test_returns_root_when_only_root_exists(self, monkeypatch): + """If every ancestor except the filesystem root is gone, the root + itself is still a valid recovery target — don't skip it just because + ``os.path.dirname('/') == '/'`` is the loop's exit condition.""" + sep = os.path.sep + monkeypatch.setattr(os.path, "isdir", lambda p: p == sep) + assert _resolve_safe_cwd("/no/such/deep/dir") == sep + def _fake_interrupt(): return threading.Event() -def _make_fake_popen(captured: dict): +def _make_fake_popen(captured: dict, fds: list): + """Build a fake ``Popen`` whose ``stdout`` exposes a real OS file + descriptor so ``BaseEnvironment._wait_for_process`` can call + ``select.select([fd], ...)`` and ``os.read(fd, ...)`` against it without + tripping ``TypeError: fileno() returned a non-integer`` from a MagicMock + ``fileno()`` (or worse, accidentally reading from the test runner's own + stdout). + + The pipe's write end is closed immediately so the drain loop sees EOF on + the first iteration. Every fd handed out is appended to ``fds`` so the + caller can clean up after the test. + """ def fake_popen(cmd, **kwargs): captured["cwd"] = kwargs.get("cwd") captured["env"] = kwargs.get("env", {}) + read_fd, write_fd = os.pipe() + os.close(write_fd) + stdout = os.fdopen(read_fd, "rb", buffering=0) + fds.append(stdout) proc = MagicMock() proc.poll.return_value = 0 proc.returncode = 0 - proc.stdout = MagicMock( - __iter__=lambda s: iter([]), - __next__=lambda s: (_ for _ in ()).throw(StopIteration), - ) + proc.stdout = stdout proc.stdin = MagicMock() return proc return fake_popen +def _close_fds(fds): + for f in fds: + try: + f.close() + except Exception: + pass + + class TestRunBashCwdRecovery: """End-to-end recovery: deleted ``self.cwd`` must not crash Popen.""" @@ -82,11 +110,15 @@ class TestRunBashCwdRecovery: assert env.cwd == str(wedged) and not os.path.isdir(env.cwd) captured = {} - with patch("tools.environments.local._find_bash", return_value="/bin/bash"), \ - patch("subprocess.Popen", side_effect=_make_fake_popen(captured)), \ - patch("tools.terminal_tool._interrupt_event", _fake_interrupt()), \ - caplog.at_level("WARNING", logger="tools.environments.local"): - env.execute("echo hello") + fds: list = [] + try: + with patch("tools.environments.local._find_bash", return_value="/bin/bash"), \ + patch("subprocess.Popen", side_effect=_make_fake_popen(captured, fds)), \ + patch("tools.terminal_tool._interrupt_event", _fake_interrupt()), \ + caplog.at_level("WARNING", logger="tools.environments.local"): + env.execute("echo hello") + finally: + _close_fds(fds) # Popen must have been handed a real, existing directory. assert captured["cwd"] == str(tmp_path) @@ -103,11 +135,15 @@ class TestRunBashCwdRecovery: env = LocalEnvironment(cwd=str(tmp_path), timeout=10) captured = {} - with patch("tools.environments.local._find_bash", return_value="/bin/bash"), \ - patch("subprocess.Popen", side_effect=_make_fake_popen(captured)), \ - patch("tools.terminal_tool._interrupt_event", _fake_interrupt()), \ - caplog.at_level("WARNING", logger="tools.environments.local"): - env.execute("echo hello") + fds: list = [] + try: + with patch("tools.environments.local._find_bash", return_value="/bin/bash"), \ + patch("subprocess.Popen", side_effect=_make_fake_popen(captured, fds)), \ + patch("tools.terminal_tool._interrupt_event", _fake_interrupt()), \ + caplog.at_level("WARNING", logger="tools.environments.local"): + env.execute("echo hello") + finally: + _close_fds(fds) assert captured["cwd"] == str(tmp_path) assert env.cwd == str(tmp_path) diff --git a/tools/environments/local.py b/tools/environments/local.py index 99a7d9d12c..72d4f04d9c 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -31,10 +31,15 @@ def _resolve_safe_cwd(cwd: str) -> str: if cwd and os.path.isdir(cwd): return cwd parent = os.path.dirname(cwd) if cwd else "" - while parent and parent != os.path.dirname(parent): + while parent: if os.path.isdir(parent): return parent - parent = os.path.dirname(parent) + next_parent = os.path.dirname(parent) + if next_parent == parent: + # Reached the filesystem root and it doesn't exist either — + # genuinely nothing to fall back to except the temp dir. + break + parent = next_parent return tempfile.gettempdir() From 109c3e468c8ffaf5733683aa1f02afb925104be4 Mon Sep 17 00:00:00 2001 From: kshitij <82637225+kshitijk4poor@users.noreply.github.com> Date: Mon, 4 May 2026 15:35:34 -0700 Subject: [PATCH 07/21] fix(terminal): guard background process spawn against deleted cwd (#19933) Follow-up to #19928 which fixed the foreground path in _run_bash. The background process spawn in process_registry.py had the same vulnerability: Popen(cwd=session.cwd) and PtyProcess.spawn(cwd=...) would raise FileNotFoundError if the directory was deleted. Apply _resolve_safe_cwd() at session creation time so both the PTY and pipe-mode Popen paths receive a validated cwd. --- tools/process_registry.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/process_registry.py b/tools/process_registry.py index da5c8d224b..0fc312185d 100644 --- a/tools/process_registry.py +++ b/tools/process_registry.py @@ -41,7 +41,7 @@ import time import uuid _IS_WINDOWS = platform.system() == "Windows" -from tools.environments.local import _find_shell, _sanitize_subprocess_env +from tools.environments.local import _find_shell, _resolve_safe_cwd, _sanitize_subprocess_env from dataclasses import dataclass, field from typing import Any, Dict, List, Optional @@ -480,7 +480,7 @@ class ProcessRegistry: command=command, task_id=task_id, session_key=session_key, - cwd=cwd or os.getcwd(), + cwd=_resolve_safe_cwd(cwd or os.getcwd()), started_at=time.time(), ) From 20428f5e600cddb414ab1ec6152aabb0497d14e1 Mon Sep 17 00:00:00 2001 From: brooklyn! Date: Mon, 4 May 2026 15:49:28 -0700 Subject: [PATCH 08/21] fix(tui): respect voice.record_key config (supersedes #19028, #19339) (#19835) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(tui): respect voice.record_key config instead of hardcoded Ctrl+B Classic CLI loaded ``voice.record_key`` from config.yaml and bound the prompt-toolkit handler dynamically (``cli.py`` paths). The new TUI hard- coded ``Ctrl+B`` everywhere — ``isVoiceToggleKey`` (input handler), ``/voice status`` ("Record key: Ctrl+B"), and ``/voice on`` ("Ctrl+B to start/stop recording"). A user who set ``voice.record_key: ctrl+o`` (or any other key) saw the documented config silently ignored — only Ctrl+B worked, the displayed shortcut lied about it. Wire the configured key end to end through the existing channels: * **Backend** (``tui_gateway/server.py``): ``voice.toggle`` action=status AND action=on/off responses now include ``record_key``, sourced from ``config.get('voice', {}).get('record_key', 'ctrl+b')``. * **Backend types** (``ui-tui/src/gatewayTypes.ts``): ``ConfigFullResponse`` now exposes ``config.voice.record_key`` and ``VoiceToggleResponse`` carries ``record_key`` so the TUI can both bind and display it. * **Frontend parser/formatter** (``ui-tui/src/lib/platform.ts``): ``parseVoiceRecordKey()`` accepts ``ctrl+b`` / ``alt+r`` / ``cmd+space`` and the common aliases (``option``, ``cmd``, ``win``, …); falls back to the documented Ctrl+B for empty / multi-character / malformed input so a typo never silently disables the shortcut. ``formatVoiceRecordKey()`` renders for status text. ``isVoiceToggleKey`` now takes a parsed ``ParsedVoiceRecordKey`` argument; the hardcoded ``ch === 'b'`` is gone. Default arg keeps existing call sites back-compat. * **Hydration** (``ui-tui/src/app/useConfigSync.ts``, ``useMainApp.ts``): startup ``config.get full`` already runs; extract ``cfg.voice.record_key`` from it, parse, push into a new ``voiceRecordKey`` state, and forward to the input handler ctx (``InputHandlerContext.voice.recordKey``). Mtime-poll path also re-applies the parsed key so a hand-edit of config.yaml takes effect the next tick — matches existing behaviour for display options. * **Input handler** (``ui-tui/src/app/useInputHandlers.ts``): ``isVoiceToggleKey(key, ch, voice.recordKey)`` so the configured binding fires. * **Slash command** (``ui-tui/src/app/slash/commands/session.ts``): ``/voice status`` and ``/voice on`` use ``formatVoiceRecordKey`` on the response's ``record_key`` instead of the hardcoded label. Tests: * ``parseVoiceRecordKey`` covers ctrl/alt/cmd/super aliases, multi-char rejection, and empty fallback. * ``formatVoiceRecordKey`` covers the doc examples (``Ctrl+B``, ``Ctrl+O``, ``Alt+R``, ``Cmd+B``). * ``isVoiceToggleKey`` regression: ``ctrl+o`` configured → only ``o`` matches, not ``b``; ``alt+r`` matches both alt-bit and meta-bit encodings (terminal protocol parity); omitted-arg call still binds Ctrl+B for back-compat. Full TUI suite (555 tests) passes; ``tsc --noEmit`` clean. Fixes #18994 Co-authored-by: asheriif * fix(tui): support named-key tokens in voice.record_key (space, enter, …) Reviewer caught that the round-1 parser in #18994 rejected every multi-character token, so a config value like ``ctrl+space`` (which the CLI happily binds via prompt_toolkit's ``c-space`` rewrite in ``cli.py``) silently fell back to the documented Ctrl+B default — re-introducing the same false-shortcut bug the PR was meant to fix, just at a different surface. Add explicit named-key support that mirrors what the CLI accepts: * ``space`` (alias: ``spc``) → matches ``ch === ' '`` * ``enter`` (alias: ``return``, ``ret``) → matches ``key.return`` * ``tab`` → matches ``key.tab`` * ``escape`` (alias: ``esc``) → matches ``key.escape`` * ``backspace`` (alias: ``bs``) → matches ``key.backspace`` * ``delete`` (alias: ``del``) → matches ``key.delete`` ``ParsedVoiceRecordKey`` gains an optional ``named`` field; ``ch`` holds either a single char (back-compat) or the canonical named token, and the runtime matcher dispatches on ``named`` before checking the modifier shape. Aliases collapse to one canonical name so ``ctrl+esc`` and ``ctrl+escape`` behave identically. Unrecognised multi-character tokens (e.g. ``ctrl+spcae`` typo, or unsupported keys like ``ctrl+f5``) still fall back to the Ctrl+B default rather than silently disabling the binding — keeps the "typo never silently kills the shortcut" guarantee. Tests: * ``parseVoiceRecordKey`` parametrised over every named token + each alias variant. * New ``isVoiceToggleKey`` cases for space (ch-based match), enter (``key.return``), tab, escape, backspace, delete, including modifier-mismatch negatives. * ``formatVoiceRecordKey`` renders named keys in title case (``Ctrl+Space``, ``Ctrl+Enter``). * Existing fall-back-to-Ctrl+B contract preserved for empty input AND unrecognised multi-char tokens. Full TUI suite: 559/559 pass; ``tsc --noEmit`` clean. Refs #18994 (round-1 review feedback) Co-authored-by: asheriif * test(tui): assert voice.toggle returns configured record_key Salvage the backend regression from #19339 — asserts ``voice.toggle`` action=on AND action=status responses carry the configured ``voice.record_key`` end-to-end through ``_load_cfg()``. Keeps the CLI→TUI parity contract visible in the Python test suite alongside the existing frontend parser/matcher/formatter coverage from #19028. * fix(tui): address Copilot review on #19835 voice.record_key wiring Five tightenings on the parser + matcher + hydration surface, all caught by the Copilot review on the PR — each one turns a silent false-fire or display/binding skew into a deterministic behaviour. * **isVoiceToggleKey ctrl branch was too permissive for named keys.** The doc-default macOS Cmd+B muscle-memory fallback (``isActionMod(key)`` on top of ``key.ctrl``) fired for every configured key, so bare Esc — which hermes-ink reports with ``key.meta`` on some macOS terminals — triggered ``ctrl+escape``, and Alt+Space / Alt+Tab triggered ``ctrl+space`` / ``ctrl+tab``. Gate the fallback to the literal ``ctrl+b`` binding so any custom chord requires the real Ctrl bit. * **Alt branch guarded against Ctrl/Cmd co-press.** Without this, Ctrl+Alt+ and Cmd+Alt+ also fired ``alt+``. * **Dropped the ``meta`` modifier variant and its alias.** In hermes-ink ``key.meta`` is Alt on xterm-style terminals and Cmd on legacy macOS ones, so a literal ``meta+b`` config displayed as ``Cmd+B`` while matching Alt+B — exactly the kind of false shortcut the PR was meant to remove. ``cmd`` / ``command`` now collapse onto ``super`` (kitty-style ``key.super``, with a macOS ``key.meta`` fallback) and render as ``Cmd+B``. Unknown modifier tokens fall back to the documented Ctrl+B default rather than silently coercing to Ctrl. * **Slash-command display/binding skew.** ``/voice status`` and ``/voice on`` rendered from the fresh gateway ``record_key`` response, but ``useInputHandlers()`` still bound the old key until the next 5s mtime poll. Thread ``setVoiceRecordKey`` through ``SlashHandlerContext.voice`` and push the parsed spec into frontend state on every response so text and binding stay consistent. * **Test coverage for the two paths Copilot flagged.** Added vitest coverage for (a) the three-case ``/voice`` slash output in ``createSlashHandler.test.ts`` and (b) the ``applyDisplay → voice.record_key`` hydration + omit-setter back-compat paths in ``useConfigSync.test.ts``. Plus regression cases for every false-fire scenario above. Suite: 575/575 green, tsc --noEmit clean. * fix(tui): address Copilot round-2 review on #19835 Three tightenings on the surface introduced in the round-1 fix: * **``/voice tts`` reset custom bindings to Ctrl+B.** The ``tts`` branch of ``voice.toggle`` omitted ``record_key`` from its response, so the frontend's ``r.record_key ?? 'ctrl+b'`` coerced a user's custom binding back to the default on every TTS toggle. Two-sided fix: the backend now includes ``record_key`` on the ``tts`` branch (parity with ``status``/``on``/``off``), and the slash handler only pushes frontend state when the response actually carries ``record_key`` — belt-and-suspenders against any future branch forgetting to include it. * **``super+b`` / ``win+b`` / ``cmd+b`` displayed "Cmd+B" on Linux and Windows.** ``formatVoiceRecordKey`` rendered ``mod === 'super'`` as ``Cmd`` universally, which told non-mac users the wrong modifier to press even though ``isVoiceToggleKey`` matched the right event bits. Gate the label to ``isMac`` so non-mac renders ``Super+B``. * **``control+b`` / ``ctrl + b`` lost the macOS Cmd+B fallback.** ``_isDefaultVoiceKey`` keyed off ``parsed.raw`` — so semantically-equal aliases of the documented default dropped into the strict branch even though they bind Ctrl+B. Compare on the parsed spec (mod + ch + named) instead. Coverage added: Linux ``Super+B`` rendering (and macOS ``Cmd+B``), ``control+b`` / ``ctrl + b`` accepting the Cmd+B fallback on darwin, ``/voice tts`` without ``record_key`` not clobbering cached binding, and a backend regression asserting every ``voice.toggle`` branch carries the configured key. Suite: 579/579 TUI vitest green, 2/2 backend voice tests green, tsc --noEmit clean. * fix(tui): address Copilot round-3 review on #19835 Three classes of robustness issue caught on the second pass — all revolve around malformed YAML tipping ``parseVoiceRecordKey`` or ``_voice_record_key`` into a crash instead of the documented fallback. * **Parser crashed on non-string YAML scalars.** ``config.get full`` returns raw ``yaml.safe_load`` output, so ``voice.record_key: 1`` or ``voice.record_key: true`` in a hand-edited config would hit ``.trim()`` on a number/bool and throw, breaking startup and every mtime re-apply. Accept ``unknown`` at the signature, guard with ``typeof raw !== 'string'``, and fall back to the default. * **Backend blew up on non-dict ``voice:``.** Same YAML hazard on the gateway side: ``voice: true`` / ``voice: cmd+b`` left ``_load_cfg().get("voice")`` as a bool/str, so ``.get("record_key")`` raised AttributeError and took every ``voice.toggle`` branch down with it. Centralised the lookup in a single ``_voice_record_key()`` helper that ``isinstance``-guards both ``voice`` and ``record_key`` and falls back to ``ctrl+b``. * **Multi-modifier chords silently dropped extras.** The previous validator only checked the first modifier token, so ``ctrl+alt+r`` silently parsed as ``ctrl+r`` and ``cmd+ctrl+b`` as ``super+b`` — a typo bound a different shortcut than the user configured. Reject multi-modifier spellings outright; the classic CLI only supports single-modifier bindings via prompt_toolkit's ``c-x`` / ``a-x`` rewrite, so this matches CLI parity. Coverage added: * ``parseVoiceRecordKey`` fallback on ``1`` / ``true`` / ``null`` / ``undefined`` / ``{}``. * ``parseVoiceRecordKey`` fallback on ``ctrl+alt+r`` / ``cmd+ctrl+b`` / ``alt+ctrl+space``. * ``test_voice_toggle_handles_non_dict_voice_cfg`` exercises every non-dict ``voice:`` shape (bool, str, None, int, list) and asserts each falls back to ``record_key: 'ctrl+b'``. Suite: 581/581 TUI vitest green, 3/3 backend voice tests green, tsc --noEmit clean. * fix(tui): address Copilot round-4 review on #19835 Four final corners of the voice.record_key surface: * **Bare-char configs silently coerced to ``ctrl+``.** A config like ``voice.record_key: o`` / ``space`` / ``escape`` fell through to the default ``mod = 'ctrl'`` and silently bound Ctrl+O, while the classic CLI's prompt_toolkit would bind the raw key (no rewrite) — so the two runtimes silently disagreed on what "o" means. Require an explicit modifier; bare-char configs fall back to the documented Ctrl+B default. * **Reserved ctrl+ bindings would never fire.** ``useInputHandlers()`` intercepts ``ctrl+c`` (interrupt), ``ctrl+d`` (quit), and ``ctrl+l`` (clear screen) before the voice check runs, so those configs would be advertised in /voice status but the advertised shortcut never actually triggers push-to-talk. Added ``_RESERVED_CTRL_CHARS`` at parse time so the user gets the documented default instead of a dead shortcut. (``alt+c``, ``cmd+l``, etc. are not intercepted and stay usable.) * **``_load_cfg()`` root itself may be a non-dict.** ``_voice_record_key()`` isinstance-guarded the ``voice`` subkey but not the root — a malformed config.yaml that collapsed to a scalar/list at the top level (``config.yaml: true`` or ``[]``) would still raise on ``.get("voice")``. Added the top-level guard too so every malformed shape falls back to ``ctrl+b``. * **Stale header comment on ``isVoiceToggleKey``.** The doc-comment still claimed "On macOS we additionally accept the platform action modifier (Cmd) for the configured letter" even though the implementation gates the Cmd fallback to the documented default only. Rewrote to match. Coverage added: * ``parseVoiceRecordKey`` fallback on bare chars (``o``, ``b``, ``space``, ``escape``). * ``parseVoiceRecordKey`` fallback on ``ctrl+c`` / ``ctrl+d`` / ``ctrl+l``; positive case for ``alt+c`` / ``cmd+l`` still usable. * Backend ``test_voice_toggle_handles_non_dict_voice_cfg`` now exercises 5 non-dict shapes at the YAML root too. Suite: 583/583 TUI vitest green, 3/3 backend voice tests green, tsc --noEmit clean. * fix(tui): address Copilot round-5 review on #19835 Three follow-ups on the voice matcher's modifier + shift discipline: * **``super`` branch falsely fired on Alt+ / bare Esc on macOS.** ``isVoiceToggleKey`` accepted ``isMac && key.meta`` as a Cmd fallback for the ``super`` modifier — but hermes-ink sets ``key.meta`` for plain Alt/Option AND for bare Escape on some macOS terminals. A ``cmd+b`` config silently fired on Alt+B; ``cmd+space`` on Alt+Space; ``cmd+escape`` on bare Esc. Drop the fallback and require the literal ``key.super`` bit. Legacy- terminal users who need Cmd should upgrade to a kitty-protocol terminal or bind ``alt+X`` explicitly. * **Shift bit was never checked.** The parser rejects multi- modifier configs like ``ctrl+shift+tab``, but the runtime matcher didn't check ``key.shift`` — so ``ctrl+tab`` also fired on Ctrl+Shift+Tab and ``alt+enter`` on Alt+Shift+Enter. Early-return on ``key.shift === true`` so the runtime only fires the exact chord the user configured. * **Test leaked ``HERMES_VOICE=1`` into later tests.** ``voice.toggle`` action=on writes to ``os.environ`` directly (CLI parity, runtime-only flag); ``test_voice_toggle_returns_ configured_record_key`` dispatched action=on without letting monkeypatch take ownership of the var first. Any later test that read voice mode in the same Python process could inherit a stale enabled state. Added ``monkeypatch.setenv("HERMES_VOICE", "0")`` up front so monkeypatch restores the original value at teardown. Coverage added: * ``cmd+b`` / ``cmd+space`` / ``cmd+escape`` do NOT fire on ``key.meta``-only events on darwin. * ``ctrl+tab`` / ``alt+enter`` / ``ctrl+o`` reject matches when ``key.shift`` is held; sanity cases without Shift still fire. Suite: 585/585 TUI vitest green, 3/3 backend voice tests green, tsc --noEmit clean. * fix(tui): address Copilot round-6 review on #19835 Three classes of modifier-discipline tightening + one config-surface honesty fix: * **Default ``ctrl+b`` Cmd fallback leaked Alt+B.** The default's macOS Cmd+B muscle-memory path used ``isActionMod(key)``, which returns ``key.meta || key.super`` on darwin. hermes-ink also reports plain Alt as ``key.meta``, so Alt+B silently fired the default binding. Replaced with strict ``isMac && key.super === true`` — kitty-style Cmd+B still works, Alt+B correctly rejected. Legacy-terminal mac users (Terminal.app without CSI-u) now get raw Ctrl+B only; the documented default still works everywhere. * **ctrl / super branches accepted extra modifier bits.** The parser rejects multi-modifier configs like ``ctrl+alt+o``, but the runtime matcher was permissive — ``ctrl+o`` fired on Ctrl+Alt+O / Ctrl+Cmd+O, and ``super+b`` fired on Cmd+Alt+B / Ctrl+Cmd+B. Added strict ``!key.alt && !key.meta && key.super !== true`` on ctrl, and ``!key.ctrl && !key.alt && !key.meta`` on super, so the runtime only fires the exact chord the parser would let you configure. * **Dropped ``cmd`` / ``command`` aliases.** They parsed to ``super`` and rendered as ``Cmd+X``, but legacy macOS terminals report Cmd as ``key.meta`` (same signal as Alt), so a ``cmd+o`` config was advertised as working but never actually fired on Terminal.app-without-CSI-u. That recreated the "displayed shortcut does not work" problem this PR was meant to remove. Users who want the platform action modifier spell it ``super`` / ``win`` — that matches the unambiguous ``key.super`` bit, and kitty-style macOS terminals render it as ``Cmd+X`` via platform-aware formatter. Coverage updated: * Default ctrl+b no longer fires on Alt+B via ``key.meta`` leak; raw Ctrl+B and kitty-style Cmd+B still fire. * ``ctrl+o`` rejects Ctrl+Alt+O / Ctrl+Cmd+O / Ctrl+Meta+O chords. * ``super+b`` rejects Cmd+Alt+B / Cmd+Meta+B / Ctrl+Cmd+B chords. * ``cmd+b`` / ``command+b`` / ``meta+b`` all fall back to the documented default at parse time (joined the ambiguous-mac-mod rejection class). * Round-2 expectations that asserted ``cmd+b`` parsed as super and accepted ``key.meta`` on darwin updated to reflect the new stricter contract. Suite: 588/588 TUI vitest green, 3/3 backend voice tests green, tsc --noEmit clean. * fix(tui): address Copilot follow-up on wire typing + escape precedence Two follow-ups from the latest Copilot pass: * **Config wire typing honesty (`gatewayTypes.ts`)** `config.get full` forwards raw `yaml.safe_load()` output, so `voice.record_key` can be any scalar/container when hand-edited. Typing it as `string` suggests a normalized contract that the backend does not guarantee and makes unsafe callers more likely. Change `ConfigVoiceConfig.record_key` to `unknown` with an explicit comment that callers must normalize at runtime. * **Escape-based voice bindings were swallowed before voice check** `useInputHandlers()` handled `key.escape` for queue-edit cancel and selection clear before `isVoiceToggleKey(...)`, so configured `ctrl+escape` / `alt+escape` / `super+escape` chords were advertised but never toggled recording in those UI states. Add an early escape+voice check before generic Esc handlers so escape-based voice bindings win when configured, while plain Esc behavior remains unchanged. Also updated PR #19835 description text to remove stale cmd/command alias claims and match the current parser contract. * fix(tui): pass configured voice shortcut through TextInput layer Thread the live parsed voiceRecordKey into TextInput so configured voice.record_key chords bubble to useInputHandlers instead of being consumed as editor input. This removes the last hardcoded Ctrl+B pass-through in the composer path while preserving existing global control chord behavior. * fix(tui): require explicit alt bit for escape-based alt chords Hermes-ink reports bare Escape as meta=true+escape=true on some terminals, so a configured alt+escape binding was firing on bare Esc. Require an explicit key.alt bit when the configured named key is escape so plain Esc stays plain Esc; kitty-style alt+escape still fires. * fix(tui): harden voice.record + TextInput paste + super-mod reserved list Three round-7 Copilot follow-ups on #19835: - voice.record start handler used _load_cfg().get('voice', {}).get(...) without shape checks, so malformed YAML (bool/scalar/list) returned 5025 instead of using VAD defaults. Centralized _voice_cfg_dict() helper and type-guarded silence_threshold/silence_duration with numeric fallbacks. - TextInput pass-through check moved above paste/copy handling so configured voice chords (ctrl+v / alt+v / cmd+v) beat the composer's paste/copy defaults. - parser now also rejects super+{c,d,l,v} — on macOS those are copy/exit/clear/paste and would be advertised in /voice status but never actually toggle recording. * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix(tui): round-8 Copilot review — allow ctrl+x, gate super reservations to macOS, preserve voice key on transient RPC failure Three round-8 Copilot follow-ups on #19835: - Revert ctrl+x addition to _RESERVED_CTRL_CHARS (landed via Copilot Autofix commit 731ec86): ctrl+x is only claimed during queue-edit (queueEditIdx !== null), so voice works the rest of the session and matches CLI ctrl+ parity. - Gate super+{c,d,l,v} reservation to isMac. Linux/Windows TUI globals key off Ctrl, so kitty/CSI-u super+ configs don't collide on non-mac and should stay usable. - applyDisplay() now skips setVoiceRecordKey when cfg is null so one transient quietRpc() failure after a config edit doesn't clobber the cached binding back to Ctrl+B until the next successful poll. New coverage: - parseVoiceRecordKey preserves ctrl+x on linux - super+{c,d,l,v} rejected on darwin, allowed on linux - applyDisplay(null, ...) leaves voiceRecordKey untouched * fix(cli,tui): normalize voice.record_key aliases across CLI + TUI for parity Round-9 Copilot review on #19835: TUI accepted control+/option+/opt+/super+/win+ aliases but the classic CLI only rewrote literal ctrl+/alt+ before handing to prompt_toolkit, so a TUI-valid config silently bound a different (or no) shortcut in the CLI. - Added normalize_voice_record_key_for_prompt_toolkit() in hermes_cli/voice.py with a single alias table (ctrl/control/alt/option/opt → c-/a-). - Wired it into all three cli.py sites (_enable_voice_mode hint, _show_voice_status display, and the prompt_toolkit binding in _register_voice_handler). - /voice status display now renders control+x as Ctrl+X and option+x as Alt+X (canonical casing) to match TUI formatVoiceRecordKey. - super/win/windows are intentionally left unchanged: prompt_toolkit has no super modifier, so the CLI will reject them loudly at startup rather than silently binding Ctrl+B. Documented this split at both the TUI _MOD_ALIASES comment and the CLI normalizer docstring. - Added tests covering ctrl/control/alt/option/opt mapping, case-insensitivity, non-string fallback, empty-string fallback, and super/win pass-through. * fix(cli): port TUI parser contract into CLI voice.record_key normalizer Round-10 Copilot review on #19835. hermes_cli/voice.py's normalize_voice_record_key_for_prompt_toolkit() previously did blind substring replacement with no trim/validate step, so the CLI diverged from the TUI parser on: - whitespace ('ctrl + b' -> 'c- b' instead of 'c-b') - typoed named keys ('ctrl+spcae' passed through as 'c-spcae' and prompt_toolkit would reject at startup) - bare-char configs ('o' should fall back, not pass through as 'o') - multi-modifier chords ('ctrl+alt+r') - reserved ctrl chars ('ctrl+c/d/l') - unknown modifiers ('meta+b' / 'shift+b') - named-key aliases ('return'/'esc'/'bs'/'del' not collapsed to prompt_toolkit canonicals) Port the TUI parser contract into Python (_VOICE_MOD_ALIASES, _VOICE_NAMED_KEYS, _VOICE_RESERVED_CTRL_CHARS) so one config value binds the same shortcut in both runtimes. Also added format_voice_record_key_for_status() shared between the PTT hint and /voice status display. Non-string scalars (voice.record_key: true / 1) now surface as 'Ctrl+B' instead of the raw scalar — /voice status no longer advertises a shortcut that can never bind. Tests: 29/29 in test_voice_wrapper.py, including 11 new regressions covering whitespace, named-key aliases, typos, bare-char, multi-modifier, reserved ctrl, unknown mods, non-string fallback, and formatter contract. * fix(cli): shape-safe voice config read + graceful super/win fallback Round-11 Copilot review on #19835. Two remaining cross-runtime gaps: 1. load_config().get('voice', {}) still assumed voice was a dict, so a hand-edited voice: true / voice: cmd+b at the top level raised AttributeError before the voice UI could start. Added voice_record_key_from_config(cfg) to hermes_cli/voice.py that isinstance-guards both the root and the voice subkey. All three cli.py read sites (_enable_voice_mode hint, _show_voice_status, PTT binding) now use it. 2. The CLI normalizer previously passed super+/win+/windows+ through unrewritten so prompt_toolkit would reject them loudly at startup — but that crash was a worse UX than a silent fallback. Normalizer now returns c-b for those spellings, and the PTT binding site logs a warning so users see why their TUI-only shortcut isn't binding in the CLI. Coverage: 34/34 in tests/hermes_cli/test_voice_wrapper.py (5 new cases for voice_record_key_from_config + malformed-root + malformed-voice + extractor/normalizer composition). * fix(cli): self-audit cleanup — remaining voice-config shape safety + doc drift Self-review of the voice.record_key change set turned up four remaining items Copilot would very likely flag next round: 1. cli.py _voice_start_continuous still read load_config().get('voice', {}).get('silence_threshold') without an isinstance guard, so a hand-edited voice: true / voice: cmd+b (non-dict) raised AttributeError on VAD recording start. Shape-safe coerce the voice dict and numeric-guard silence_threshold/silence_duration. 2. cli.py _enable_voice_mode's auto_tts check had the same bug — fixed with the same isinstance guard. 3. hermes_cli/voice.py module comment on _VOICE_MOD_ALIASES still said super/win/windows 'pass through unchanged and prompt_toolkit's add() call loudly rejects them at startup'. Round 11 changed the normalizer to silently fall back to c-b with a warning at the binding site; updated the comment to match. 4. ui-tui/src/lib/platform.ts header comment had the same stale 'CLI will loudly reject them at startup' claim; updated to 'falls back to the documented default and logs a warning'. No behavior change on the code paths already covered by test_voice_wrapper.py; the two cli.py fixes are defensive against malformed YAML that previous rounds already hardened in tui_gateway/server.py but missed in the classic CLI. * fix(cli,tui): round-12 Copilot review — alt-collide on mac, bool-in-int guards, voice UI hardcodes, mtime-reload test Five round-12 Copilot review items on #19835: 1. platform.ts: hermes-ink reports Alt as key.meta on many terminals; isActionMod on darwin accepts key.meta as the action modifier. So alt+c/d/l get claimed by isCopyShortcut / isAction('d')/'l') before the voice check. Reject those configs at parse time on macOS only (non-mac keeps them usable). 2. cli.py: four remaining hardcoded 'Ctrl+B' sites in voice-facing UI (_get_voice_status_fragments status bar, _voice_start_recording hints, _get_placeholder composer text) were still lying about non-default configs. Added self._voice_record_key_label() shared helper and wired it into all three sites. 3. server.py + cli.py: bool is a subclass of int, so isinstance(silence_threshold, (int, float)) accepted True/False from malformed YAML and forwarded 1/0 to the VAD engine. Exclude bool explicitly so boolean typos fall back to the documented 200 / 3.0 defaults. 4. useConfigSync.ts: extracted the config.get-full fetch+apply body into a shared hydrateFullConfig() helper. Both the initial hydration and mtime-reload paths now use it, so the polling/RPC wiring is exercised by direct unit tests (4 new cases: fresh apply, reapply on new value, transient RPC failure preserves cache, back-compat without voice setter). 5. Added alt+{c,d,l} rejection regressions on darwin + allow on linux, and bool-leak regressions for both silence_threshold and silence_duration in tests/test_tui_gateway_server.py. Suite: 602/602 TUI vitest, 38/38 backend voice tests, typecheck + lints clean. * fix(cli): cache voice record-key label at binding time + status-bar coverage Round-13 Copilot review on #19835. _voice_record_key_label() was reading live config on every render, which caused two problems: 1. prompt_toolkit registers the push-to-talk binding once at session start (@kb.add(_voice_key)); the binding does NOT re-read config. Editing voice.record_key mid-session would switch the status-bar / placeholder / recording-hint label to the new shortcut while the actual keybinding stayed on the startup chord — reintroducing the display/binding drift this whole PR is fighting. 2. Hot render path: during recording the UI is invalidated every 150ms, so re-loading + deep-merging config on every call added avoidable UI overhead. Fix: cache the label at the same site that registers the prompt_toolkit binding via new set_voice_record_key_cache(raw_key). _voice_record_key_label() now just returns the cached value (falls back to 'Ctrl+B' before startup). Status/placeholder/hint are always in sync with the live binding; no config reload per render. Also added 4 regression cases to tests/cli/test_cli_status_bar.py: configured ctrl+ renders in both wide and compact status bars, configured named key (ctrl+space) renders in the recording hint, pre-startup absent cache falls back to Ctrl+B, and malformed configs (bool True) fall through the formatter to Ctrl+B. Suite: 60/60 test_cli_status_bar + test_voice_wrapper, typecheck + lints clean. * fix(cli): route /voice on + /voice status through startup-pinned label; mac alt+cdl parity Round-14 Copilot review on #19835. All three comments legit: 1. _enable_voice_mode still formatted label from live load_config() — mid-session config edit would make /voice on announce the new shortcut while the prompt_toolkit binding stayed the startup chord. Use self._voice_record_key_label() (cached at binding time, round-13) so /voice on cannot drift from the live binding. 2. _show_voice_status had the same bug — /voice status reported live config instead of the pinned startup binding. Fixed the same way. 3. CLI normalizer accepted alt+c/alt+d/alt+l even though the TUI parser rejects them on macOS (Copilot round-12 — hermes-ink reports Alt as key.meta, isActionMod on darwin accepts it, collides with isCopyShortcut / isAction). Added _VOICE_RESERVED_ALT_CHARS_MAC = {c,d,l} gated to sys.platform == 'darwin' so a shared config like option+c falls back to c-b on both runtimes on macOS; non-mac still binds a-c. Coverage: 4 new tests in test_voice_wrapper.py covering mac alt+cdl rejection, linux alt+cdl allowed, option/opt alias forms, and mac-specific exclusions for other alt letters. 62/62 in voice wrapper + status bar suites. --------- Co-authored-by: Tranquil-Flow Co-authored-by: asheriif Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- cli.py | 149 ++++-- hermes_cli/voice.py | 186 ++++++++ tests/cli/test_cli_status_bar.py | 62 +++ tests/hermes_cli/test_voice_wrapper.py | 237 +++++++++ tests/test_tui_gateway_server.py | 167 +++++++ tui_gateway/server.py | 68 ++- .../src/__tests__/createSlashHandler.test.ts | 61 ++- ui-tui/src/__tests__/platform.test.ts | 451 +++++++++++++++++- .../__tests__/textInputPassThrough.test.ts | 43 ++ ui-tui/src/__tests__/useConfigSync.test.ts | 140 +++++- ui-tui/src/app/interfaces.ts | 4 + ui-tui/src/app/slash/commands/session.ts | 31 +- ui-tui/src/app/useConfigSync.ts | 61 ++- ui-tui/src/app/useInputHandlers.ts | 16 +- ui-tui/src/app/useMainApp.ts | 13 +- ui-tui/src/components/appLayout.tsx | 1 + ui-tui/src/components/textInput.tsx | 50 +- ui-tui/src/gatewayTypes.ts | 9 +- ui-tui/src/lib/platform.ts | 362 +++++++++++++- 19 files changed, 2027 insertions(+), 84 deletions(-) create mode 100644 ui-tui/src/__tests__/textInputPassThrough.test.ts diff --git a/cli.py b/cli.py index eefd14fe47..e5af3c88f2 100644 --- a/cli.py +++ b/cli.py @@ -2579,23 +2579,59 @@ class HermesCLI: return f" {txt} ({elapsed_str})" return f" {txt}" + def _voice_record_key_label(self) -> str: + """Return the configured voice push-to-talk key formatted for UI. + + Shared helper so every voice-facing status line / placeholder / + recording hint advertises the SAME label as the registered + prompt_toolkit binding. + + Cached at startup (see ``set_voice_record_key_cache``) rather + than re-read per render. Two reasons (Copilot round-13 on + #19835): + + * The prompt_toolkit binding is registered once at session + start via ``@kb.add(_voice_key)``; re-reading config per + render meant the status bar could advertise a new shortcut + after a config edit while the actual binding was still the + startup chord — exactly the display/binding drift this PR + is trying to eliminate. + * The label is on the hot render path (status bar + composer + placeholder invalidated every 150ms during recording), so + reading config on every call added avoidable UI overhead. + """ + return getattr(self, "_voice_record_key_display_cache", None) or "Ctrl+B" + + def set_voice_record_key_cache(self, raw_key: object) -> None: + """Populate the voice label cache from a raw ``voice.record_key``. + + Called at CLI startup after the prompt_toolkit binding is + registered so the cached label always matches the live binding. + """ + try: + from hermes_cli.voice import format_voice_record_key_for_status + self._voice_record_key_display_cache = format_voice_record_key_for_status(raw_key) + except Exception: + self._voice_record_key_display_cache = "Ctrl+B" + def _get_voice_status_fragments(self, width: Optional[int] = None): """Return the voice status bar fragments for the interactive TUI.""" width = width or self._get_tui_terminal_width() compact = self._use_minimal_tui_chrome(width=width) + label = self._voice_record_key_label() if self._voice_recording: if compact: return [("class:voice-status-recording", " ● REC ")] - return [("class:voice-status-recording", " ● REC Ctrl+B to stop ")] + return [("class:voice-status-recording", f" ● REC {label} to stop ")] if self._voice_processing: if compact: return [("class:voice-status", " ◉ STT ")] return [("class:voice-status", " ◉ Transcribing... ")] if compact: - return [("class:voice-status", " 🎤 Ctrl+B ")] + return [("class:voice-status", f" 🎤 {label} ")] tts = " | TTS on" if self._voice_tts else "" cont = " | Continuous" if self._voice_continuous else "" - return [("class:voice-status", f" 🎤 Voice mode{tts}{cont} — Ctrl+B to record ")] + return [("class:voice-status", f" 🎤 Voice mode{tts}{cont} — {label} to record ")] def _build_status_bar_text(self, width: Optional[int] = None) -> str: """Return a compact one-line session status string for the TUI footer.""" @@ -8270,20 +8306,38 @@ class HermesCLI: return self._voice_recording = True - # Load silence detection params from config - voice_cfg = {} + # Load silence detection params from config. Shape-safe: a + # hand-edited ``voice: true`` / ``voice: cmd+b`` leaves + # ``load_config()['voice']`` as a non-dict; coerce to {} so + # continuous recording falls back to the documented defaults + # instead of crashing on ``.get()``. + voice_cfg: dict = {} try: from hermes_cli.config import load_config - voice_cfg = load_config().get("voice", {}) + _cfg = load_config().get("voice") + voice_cfg = _cfg if isinstance(_cfg, dict) else {} except Exception: pass if self._voice_recorder is None: self._voice_recorder = create_audio_recorder() - # Apply config-driven silence params - self._voice_recorder._silence_threshold = voice_cfg.get("silence_threshold", 200) - self._voice_recorder._silence_duration = voice_cfg.get("silence_duration", 3.0) + # Apply config-driven silence params (numeric-guarded so YAML + # scalar corruption doesn't break recording start-up). + # + # ``bool`` is explicitly excluded from the numeric check — in + # Python bool is a subclass of int, so a hand-edited + # ``silence_threshold: true`` would otherwise be forwarded as + # ``1`` instead of falling back to the 200 default (Copilot + # round-12 on #19835). + _threshold = voice_cfg.get("silence_threshold") + _duration = voice_cfg.get("silence_duration") + self._voice_recorder._silence_threshold = ( + _threshold if isinstance(_threshold, (int, float)) and not isinstance(_threshold, bool) else 200 + ) + self._voice_recorder._silence_duration = ( + _duration if isinstance(_duration, (int, float)) and not isinstance(_duration, bool) else 3.0 + ) def _on_silence(): """Called by AudioRecorder when silence is detected after speech.""" @@ -8309,12 +8363,13 @@ class HermesCLI: with self._voice_lock: self._voice_recording = False raise + _label = self._voice_record_key_label() if getattr(self._voice_recorder, "supports_silence_autostop", True): - _recording_hint = "auto-stops on silence | Ctrl+B to stop & exit continuous" + _recording_hint = f"auto-stops on silence | {_label} to stop & exit continuous" elif _is_termux_environment(): - _recording_hint = "Termux:API capture | Ctrl+B to stop" + _recording_hint = f"Termux:API capture | {_label} to stop" else: - _recording_hint = "Ctrl+B to stop" + _recording_hint = f"{_label} to stop" _cprint(f"\n{_ACCENT}● Recording...{_RST} {_DIM}({_recording_hint}){_RST}") # Periodically refresh prompt to update audio level indicator @@ -8559,10 +8614,12 @@ class HermesCLI: with self._voice_lock: self._voice_mode = True - # Check config for auto_tts + # Check config for auto_tts (shape-safe — malformed ``voice:`` YAML + # leaves ``voice_config`` as a non-dict, so guard before .get()). try: from hermes_cli.config import load_config - voice_config = load_config().get("voice", {}) + _raw_voice = load_config().get("voice") + voice_config = _raw_voice if isinstance(_raw_voice, dict) else {} if voice_config.get("auto_tts", False): with self._voice_lock: self._voice_tts = True @@ -8574,13 +8631,11 @@ class HermesCLI: # _voice_message_prefix property and its usage in _process_message(). tts_status = " (TTS enabled)" if self._voice_tts else "" - try: - from hermes_cli.config import load_config - _raw_ptt = load_config().get("voice", {}).get("record_key", "ctrl+b") - _ptt_key = _raw_ptt.lower().replace("ctrl+", "c-").replace("alt+", "a-") - except Exception: - _ptt_key = "c-b" - _ptt_display = _ptt_key.replace("c-", "Ctrl+").upper() + # Use the startup-pinned cache so the advertised shortcut always + # matches the live prompt_toolkit binding — reading live config + # here would drift after a mid-session config edit (Copilot + # round-14 on #19835, same class as round-13). + _ptt_display = self._voice_record_key_label() _cprint(f"\n{_ACCENT}Voice mode enabled{tts_status}{_RST}") _cprint(f" {_DIM}{_ptt_display} to start/stop recording{_RST}") _cprint(f" {_DIM}/voice tts to toggle speech output{_RST}") @@ -8637,7 +8692,6 @@ class HermesCLI: def _show_voice_status(self): """Show current voice mode status.""" - from hermes_cli.config import load_config from tools.voice_mode import check_voice_requirements reqs = check_voice_requirements() @@ -8646,9 +8700,11 @@ class HermesCLI: _cprint(f" Mode: {'ON' if self._voice_mode else 'OFF'}") _cprint(f" TTS: {'ON' if self._voice_tts else 'OFF'}") _cprint(f" Recording: {'YES' if self._voice_recording else 'no'}") - _raw_key = load_config().get("voice", {}).get("record_key", "ctrl+b") - _display_key = _raw_key.replace("ctrl+", "Ctrl+").upper() if "ctrl+" in _raw_key.lower() else _raw_key - _cprint(f" Record key: {_display_key}") + # Display the startup-pinned label so /voice status always + # matches the live prompt_toolkit binding (Copilot round-14 on + # #19835, same class as round-13). Reading live config here + # would drift after a mid-session config edit. + _cprint(f" Record key: {self._voice_record_key_label()}") _cprint(f"\n {_BOLD}Requirements:{_RST}") for line in reqs["details"].split("\n"): _cprint(f" {line}") @@ -10622,15 +10678,44 @@ class HermesCLI: run_in_terminal(_suspend) # Voice push-to-talk key: configurable via config.yaml (voice.record_key) - # Default: Ctrl+B (avoids conflict with Ctrl+R readline reverse-search) - # Config uses "ctrl+b" format; prompt_toolkit expects "c-b" format. + # Default: Ctrl+B (avoids conflict with Ctrl+R readline reverse-search). + # Config spellings (ctrl/control/alt/option/opt) are normalized to + # prompt_toolkit's c-x / a-x format via ``normalize_voice_record_key_for_prompt_toolkit`` + # so the same config value binds identically in the TUI and CLI + # (Copilot round-9 review on #19835). ``super``/``win``/``windows`` + # configs silently fall back to the default here since prompt_toolkit + # has no super modifier — log a warning so users notice the + # TUI/CLI split instead of a silent mismatch (round-11). + _raw_key: object = "ctrl+b" try: from hermes_cli.config import load_config - _raw_key = load_config().get("voice", {}).get("record_key", "ctrl+b") - _voice_key = _raw_key.lower().replace("ctrl+", "c-").replace("alt+", "a-") + from hermes_cli.voice import ( + normalize_voice_record_key_for_prompt_toolkit, + voice_record_key_from_config, + ) + _raw_key = voice_record_key_from_config(load_config()) + _voice_key = normalize_voice_record_key_for_prompt_toolkit(_raw_key) + if ( + isinstance(_raw_key, str) + and _raw_key.strip().lower().split("+", 1)[0].strip() in {"super", "win", "windows"} + and _voice_key == "c-b" + ): + logger.warning( + "voice.record_key %r uses a TUI-only modifier (super/win); " + "CLI fell back to Ctrl+B. Use ctrl+ or alt+ for " + "cross-runtime parity.", + _raw_key, + ) except Exception: _voice_key = "c-b" + # Cache the UI label here — same ``_raw_key`` that drives the + # prompt_toolkit binding below. Every status / placeholder / + # recording-hint render reads this cached value so display can + # never drift from the live keybinding even if the user edits + # voice.record_key mid-session (Copilot round-13 on #19835). + self.set_voice_record_key_cache(_raw_key) + @kb.add(_voice_key) def handle_voice_record(event): """Toggle voice recording when voice mode is active. @@ -10933,7 +11018,8 @@ class HermesCLI: def _get_placeholder(): if cli_ref._voice_recording: - return "recording... Ctrl+B to stop, Ctrl+C to cancel" + _label = cli_ref._voice_record_key_label() + return f"recording... {_label} to stop, Ctrl+C to cancel" if cli_ref._voice_processing: return "transcribing..." if cli_ref._sudo_state: @@ -10953,7 +11039,8 @@ class HermesCLI: if cli_ref._agent_running: return "msg=interrupt · /queue · /bg · /steer · Ctrl+C cancel" if cli_ref._voice_mode: - return "type or Ctrl+B to record" + _label = cli_ref._voice_record_key_label() + return f"type or {_label} to record" return "" input_area.control.input_processors.append(_PlaceholderProcessor(_get_placeholder)) diff --git a/hermes_cli/voice.py b/hermes_cli/voice.py index 0a355ce4fa..f85f30c7bf 100644 --- a/hermes_cli/voice.py +++ b/hermes_cli/voice.py @@ -27,6 +27,192 @@ import sys import threading from typing import Any, Callable, Optional +# Modifier aliases mirrored from the TUI parser (``ui-tui/src/lib/platform.ts``) +# ``_MOD_ALIASES`` table — the contract that removes the cross-runtime +# mismatch Copilot flagged in round-9 on #19835. +# +# ``super``/``win``/``windows`` are intentionally absent: prompt_toolkit +# has no super/meta modifier for the Cmd key, so those spellings are +# TUI-only. The normalizer below returns the documented default +# (``c-b``) for them — a silent fallback was preferred to a hard +# startup crash (Copilot round-11). The CLI binding site +# (``_register_voice_handler`` in cli.py) logs a warning when that +# fallback fires so users see why their TUI-only shortcut isn't +# bound in the classic CLI. +_VOICE_MOD_ALIASES = { + "ctrl": "c-", + "control": "c-", + "alt": "a-", + "option": "a-", + "opt": "a-", +} + +# Named keys prompt_toolkit accepts in ``c-`` / ``a-`` form. +# Aliases collapse to prompt_toolkit's canonical spelling so the same +# config value binds identically in both runtimes (Copilot round-10 on +# #19835). +_VOICE_NAMED_KEYS = { + "space": "space", + "spc": "space", + "enter": "enter", + "return": "enter", + "ret": "enter", + "tab": "tab", + "escape": "escape", + "esc": "escape", + "backspace": "backspace", + "bs": "backspace", + "delete": "delete", + "del": "delete", +} + +# ``useInputHandlers()`` intercepts these before the voice check runs, +# so a binding like ``ctrl+c`` (interrupt), ``ctrl+d`` (quit), or +# ``ctrl+l`` (clear screen) would be advertised in /voice status but +# never fire push-to-talk — the same blocklist the TUI parser uses. +_VOICE_RESERVED_CTRL_CHARS = frozenset({"c", "d", "l"}) + +# On macOS the classic CLI's prompt_toolkit bindings for copy / exit / +# clear also claim ``a-c`` / ``a-d`` / ``a-l`` via the action-modifier +# lookup, and hermes-ink reports Alt as ``key.meta`` on many terminals. +# Mirror the TUI parser's darwin-only reservation so ``option+c`` etc. +# don't bind Alt+C in the CLI while the TUI silently falls back to +# Ctrl+B (Copilot round-14 on #19835). +_VOICE_RESERVED_ALT_CHARS_MAC = frozenset({"c", "d", "l"}) + +_DEFAULT_PT_KEY = "c-b" + + +def voice_record_key_from_config(cfg: Any) -> Any: + """Shape-safe ``cfg.voice.record_key`` lookup. + + ``load_config()`` deep-merges raw YAML and preserves scalar + overrides, so a hand-edited ``voice: true`` / ``voice: cmd+b`` + leaves ``cfg["voice"]`` as a bool/str instead of a dict, and the + naive ``.get("voice", {}).get("record_key")`` chain raises + AttributeError before voice can even start (Copilot round-11 on + #19835). Return ``None`` for malformed shapes so call sites can + feed the result straight into the normalizer/formatter and get + the documented default. + """ + if not isinstance(cfg, dict): + return None + + voice = cfg.get("voice") + if not isinstance(voice, dict): + return None + + return voice.get("record_key") + + +def normalize_voice_record_key_for_prompt_toolkit(raw: Any) -> str: + """Coerce ``voice.record_key`` into prompt_toolkit's ``c-x`` / ``a-x`` format. + + Mirrors the TUI parser contract (``ui-tui/src/lib/platform.ts``) + so one config value binds the same shortcut in both runtimes: + + * non-string / empty / typo'd / bare-char / multi-modifier / reserved + ``ctrl+c|d|l`` → documented default ``c-b`` + * single-char keys: ``ctrl+o`` → ``c-o`` + * named keys: ``ctrl+space`` → ``c-space`` (aliases collapse: + ``ctrl+return`` → ``c-enter``) + * ``super`` / ``win`` / ``windows`` → ``c-b`` (TUI-only modifiers — + prompt_toolkit has no super mod; the CLI binding site is + expected to warn when this fallback fires so users see the + cross-runtime split, Copilot round-11 on #19835) + """ + if not isinstance(raw, str): + return _DEFAULT_PT_KEY + + lowered = raw.strip().lower() + if not lowered: + return _DEFAULT_PT_KEY + + parts = [p.strip() for p in lowered.split("+") if p.strip()] + if not parts: + return _DEFAULT_PT_KEY + + # Multi-modifier chords like ``ctrl+alt+r`` bind different shortcuts + # in prompt_toolkit (a-c-r form) and hermes-ink rejects them; collapse + # to the documented default instead of silently diverging. + if len(parts) > 2: + return _DEFAULT_PT_KEY + + # Bare char / bare named key (no explicit modifier) — the CLI's + # prompt_toolkit binds the raw key without a modifier, which the TUI + # parser refuses; reject here too so both runtimes agree. + if len(parts) == 1: + return _DEFAULT_PT_KEY + + modifier_token, key_token = parts + + # ``super`` / ``win`` / ``windows`` are TUI-only (prompt_toolkit has + # no super modifier, so ``@kb.add(super+b)`` crashes the CLI at + # startup). Fall back to the documented default here; the CLI + # binding site is expected to log a warning when the configured + # value is one of these spellings so users know the TUI+CLI + # runtimes diverge on that shortcut (Copilot round-11 on #19835). + if modifier_token in {"super", "win", "windows"}: + return _DEFAULT_PT_KEY + + normalized_mod = _VOICE_MOD_ALIASES.get(modifier_token) + if not normalized_mod: + return _DEFAULT_PT_KEY + + # Single-char key: reject reserved-ctrl chords that the TUI would + # also block at parse time, plus the mac-only alt reservation. + if len(key_token) == 1: + if normalized_mod == "c-" and key_token in _VOICE_RESERVED_CTRL_CHARS: + return _DEFAULT_PT_KEY + if ( + normalized_mod == "a-" + and sys.platform == "darwin" + and key_token in _VOICE_RESERVED_ALT_CHARS_MAC + ): + return _DEFAULT_PT_KEY + return f"{normalized_mod}{key_token}" + + # Multi-char key token must be a known named key; typos like + # ``ctrl+spcae`` fall back to the default rather than being passed + # through as ``c-spcae`` (which prompt_toolkit would reject). + named = _VOICE_NAMED_KEYS.get(key_token) + if not named: + return _DEFAULT_PT_KEY + + return f"{normalized_mod}{named}" + + +def format_voice_record_key_for_status(raw: Any) -> str: + """Render ``voice.record_key`` for ``/voice status`` in CLI-friendly form. + + Mirrors the TUI's ``formatVoiceRecordKey``: returns ``Ctrl+B`` / + ``Alt+Space`` / ``Ctrl+Enter``. Malformed configs surface as the + documented default so status never advertises a shortcut that + won't bind (Copilot round-10 on #19835). + """ + normalized = normalize_voice_record_key_for_prompt_toolkit(raw) + + if normalized.startswith("c-"): + prefix, key = "Ctrl+", normalized[2:] + elif normalized.startswith("a-"): + prefix, key = "Alt+", normalized[2:] + elif "+" in normalized: + # ``super+`` / ``win+`` — CLI won't bind them, but + # render in title case so status output is still readable. + mod, key = normalized.split("+", 1) + prefix = mod[0].upper() + mod[1:] + "+" + else: + return "Ctrl+B" + + if not key: + return prefix.rstrip("+") + + if len(key) == 1: + return prefix + key.upper() + + return prefix + key[0].upper() + key[1:] + + from tools.voice_mode import ( create_audio_recorder, is_whisper_hallucination, diff --git a/tests/cli/test_cli_status_bar.py b/tests/cli/test_cli_status_bar.py index 4a65c6e467..f5c18bfc4d 100644 --- a/tests/cli/test_cli_status_bar.py +++ b/tests/cli/test_cli_status_bar.py @@ -266,6 +266,68 @@ class TestCLIStatusBar: assert fragments == [("class:voice-status-recording", " ● REC ")] + # Round-13 Copilot review regressions on #19835. The label in voice + # status bar / recording hint / placeholder must render the + # configured ``voice.record_key`` — not hardcoded Ctrl+B. Pinning + # the cache (``set_voice_record_key_cache``) keeps display in sync + # with the prompt_toolkit binding without re-reading config on + # every render. + def test_voice_status_bar_renders_configured_ctrl_letter(self): + cli_obj = _make_cli() + cli_obj._voice_mode = True + cli_obj._voice_recording = False + cli_obj._voice_processing = False + cli_obj._voice_tts = False + cli_obj._voice_continuous = False + cli_obj.set_voice_record_key_cache("ctrl+o") + + wide = cli_obj._get_voice_status_fragments(width=120) + assert any("Ctrl+O to record" in text for _cls, text in wide) + + compact = cli_obj._get_voice_status_fragments(width=50) + assert compact == [("class:voice-status", " 🎤 Ctrl+O ")] + + def test_voice_recording_status_bar_renders_configured_named_key(self): + cli_obj = _make_cli() + cli_obj._voice_mode = True + cli_obj._voice_recording = True + cli_obj._voice_processing = False + cli_obj.set_voice_record_key_cache("ctrl+space") + + fragments = cli_obj._get_voice_status_fragments(width=120) + + assert fragments == [("class:voice-status-recording", " ● REC Ctrl+Space to stop ")] + + def test_voice_status_bar_falls_back_to_ctrl_b_without_cache(self): + cli_obj = _make_cli() + cli_obj._voice_mode = True + cli_obj._voice_recording = False + cli_obj._voice_processing = False + cli_obj._voice_tts = False + cli_obj._voice_continuous = False + # No cache set — mirrors pre-startup state; fall back to + # documented Ctrl+B default (Copilot round-13 review). + + compact = cli_obj._get_voice_status_fragments(width=50) + + assert compact == [("class:voice-status", " 🎤 Ctrl+B ")] + + def test_voice_status_bar_renders_malformed_config_as_default(self): + cli_obj = _make_cli() + cli_obj._voice_mode = True + cli_obj._voice_recording = False + cli_obj._voice_processing = False + cli_obj._voice_tts = False + cli_obj._voice_continuous = False + # Non-string / typoed configs fall through the formatter to the + # documented default so the status bar never advertises an + # invalid shortcut. + cli_obj.set_voice_record_key_cache(True) + + compact = cli_obj._get_voice_status_fragments(width=50) + + assert compact == [("class:voice-status", " 🎤 Ctrl+B ")] + class TestCLIUsageReport: def test_show_usage_includes_estimated_cost(self, capsys): diff --git a/tests/hermes_cli/test_voice_wrapper.py b/tests/hermes_cli/test_voice_wrapper.py index a372c1194f..3caacf4313 100644 --- a/tests/hermes_cli/test_voice_wrapper.py +++ b/tests/hermes_cli/test_voice_wrapper.py @@ -31,6 +31,243 @@ class TestPublicAPI: assert callable(speak_text) +class TestNormalizeVoiceRecordKeyForPromptToolkit: + """Round-9 Copilot review regression on #19835. + + Classic CLI only normalized ``ctrl+`` / ``alt+``, so TUI-valid + aliases like ``control+``, ``option+``, ``opt+`` silently bound a + different (or no) shortcut in the CLI. Normalizer now maps the + same set of aliases the TUI parser accepts, so one config value + binds identically in both runtimes. + """ + + def test_ctrl_and_alt_map_to_prompt_toolkit_form(self): + from hermes_cli.voice import normalize_voice_record_key_for_prompt_toolkit + + assert normalize_voice_record_key_for_prompt_toolkit("ctrl+b") == "c-b" + assert normalize_voice_record_key_for_prompt_toolkit("alt+r") == "a-r" + + def test_control_option_opt_aliases_match_tui_parser(self): + from hermes_cli.voice import normalize_voice_record_key_for_prompt_toolkit + + assert normalize_voice_record_key_for_prompt_toolkit("control+o") == "c-o" + assert normalize_voice_record_key_for_prompt_toolkit("option+space") == "a-space" + assert normalize_voice_record_key_for_prompt_toolkit("opt+enter") == "a-enter" + + def test_case_insensitive(self): + from hermes_cli.voice import normalize_voice_record_key_for_prompt_toolkit + + assert normalize_voice_record_key_for_prompt_toolkit("Ctrl+B") == "c-b" + assert normalize_voice_record_key_for_prompt_toolkit("CONTROL+O") == "c-o" + + def test_non_string_falls_back_to_default(self): + from hermes_cli.voice import normalize_voice_record_key_for_prompt_toolkit + + assert normalize_voice_record_key_for_prompt_toolkit(None) == "c-b" + assert normalize_voice_record_key_for_prompt_toolkit(1) == "c-b" + assert normalize_voice_record_key_for_prompt_toolkit(True) == "c-b" + assert normalize_voice_record_key_for_prompt_toolkit({}) == "c-b" + + def test_empty_string_falls_back(self): + from hermes_cli.voice import normalize_voice_record_key_for_prompt_toolkit + + assert normalize_voice_record_key_for_prompt_toolkit("") == "c-b" + + def test_super_win_fall_back_to_default_in_cli(self): + """prompt_toolkit has no super modifier, so ``super+b`` / ``win+o`` + would crash the classic CLI at startup if passed through. Fall + back to the documented default; the CLI binding site is + expected to warn so users know the shortcut is TUI-only + (Copilot round-11 on #19835).""" + from hermes_cli.voice import normalize_voice_record_key_for_prompt_toolkit + + assert normalize_voice_record_key_for_prompt_toolkit("super+b") == "c-b" + assert normalize_voice_record_key_for_prompt_toolkit("win+o") == "c-b" + assert normalize_voice_record_key_for_prompt_toolkit("windows+o") == "c-b" + + # Round-10 Copilot review regressions on #19835. + def test_strips_whitespace_within_and_around(self): + """``ctrl + b`` / `` option + space `` are accepted by the TUI + parser; the CLI normalizer must mirror that or the same config + binds different shortcuts across runtimes.""" + from hermes_cli.voice import normalize_voice_record_key_for_prompt_toolkit + + assert normalize_voice_record_key_for_prompt_toolkit("ctrl + b") == "c-b" + assert normalize_voice_record_key_for_prompt_toolkit(" option + space ") == "a-space" + + def test_named_key_aliases_collapse_to_prompt_toolkit_canonical(self): + """TUI accepts ``return`` / ``esc`` / ``bs`` / ``del`` etc.; + CLI must collapse to prompt_toolkit's canonical spelling + (``enter`` / ``escape`` / ``backspace`` / ``delete``).""" + from hermes_cli.voice import normalize_voice_record_key_for_prompt_toolkit + + assert normalize_voice_record_key_for_prompt_toolkit("ctrl+return") == "c-enter" + assert normalize_voice_record_key_for_prompt_toolkit("ctrl+esc") == "c-escape" + assert normalize_voice_record_key_for_prompt_toolkit("ctrl+bs") == "c-backspace" + assert normalize_voice_record_key_for_prompt_toolkit("alt+del") == "a-delete" + + def test_typoed_named_keys_fall_back_to_default(self): + """``ctrl+spcae`` would otherwise pass through as ``c-spcae`` and + prompt_toolkit would reject it at startup — fall back instead.""" + from hermes_cli.voice import normalize_voice_record_key_for_prompt_toolkit + + assert normalize_voice_record_key_for_prompt_toolkit("ctrl+spcae") == "c-b" + assert normalize_voice_record_key_for_prompt_toolkit("ctrl+f5") == "c-b" + + def test_bare_char_and_multi_modifier_fall_back(self): + """TUI parser rejects bare-char (``o``) and multi-modifier + (``ctrl+alt+r``) configs; the CLI normalizer must match.""" + from hermes_cli.voice import normalize_voice_record_key_for_prompt_toolkit + + assert normalize_voice_record_key_for_prompt_toolkit("o") == "c-b" + assert normalize_voice_record_key_for_prompt_toolkit("b") == "c-b" + assert normalize_voice_record_key_for_prompt_toolkit("ctrl+alt+r") == "c-b" + + def test_reserved_ctrl_chars_fall_back(self): + """``ctrl+c`` / ``ctrl+d`` / ``ctrl+l`` are always claimed by + the CLI's prompt_toolkit input layer or terminal driver; match + the TUI parser's rejection to keep /voice status honest.""" + from hermes_cli.voice import normalize_voice_record_key_for_prompt_toolkit + + assert normalize_voice_record_key_for_prompt_toolkit("ctrl+c") == "c-b" + assert normalize_voice_record_key_for_prompt_toolkit("ctrl+d") == "c-b" + assert normalize_voice_record_key_for_prompt_toolkit("ctrl+l") == "c-b" + + def test_unknown_modifier_falls_back(self): + """``meta+b`` is ambiguous on the wire (Alt on xterm, Cmd on + legacy macOS), same class as the TUI parser's rejection.""" + from hermes_cli.voice import normalize_voice_record_key_for_prompt_toolkit + + assert normalize_voice_record_key_for_prompt_toolkit("meta+b") == "c-b" + assert normalize_voice_record_key_for_prompt_toolkit("shift+b") == "c-b" + + # Round-14 Copilot review regression on #19835. On macOS the TUI + # parser rejects alt+c/d/l because hermes-ink reports Alt as + # ``key.meta`` and isActionMod(darwin) accepts it. The CLI + # normalizer must mirror that platform-gated rejection so shared + # configs like ``option+c`` don't bind Alt+C in the CLI while the + # TUI falls back to Ctrl+B. + def test_alt_cdl_rejected_on_macos(self, monkeypatch): + monkeypatch.setattr("sys.platform", "darwin") + + from hermes_cli.voice import normalize_voice_record_key_for_prompt_toolkit + + assert normalize_voice_record_key_for_prompt_toolkit("alt+c") == "c-b" + assert normalize_voice_record_key_for_prompt_toolkit("alt+d") == "c-b" + assert normalize_voice_record_key_for_prompt_toolkit("alt+l") == "c-b" + assert normalize_voice_record_key_for_prompt_toolkit("option+c") == "c-b" + assert normalize_voice_record_key_for_prompt_toolkit("opt+d") == "c-b" + # Other alt letters still bind on darwin. + assert normalize_voice_record_key_for_prompt_toolkit("alt+r") == "a-r" + assert normalize_voice_record_key_for_prompt_toolkit("alt+space") == "a-space" + + def test_alt_cdl_allowed_on_non_macos(self, monkeypatch): + monkeypatch.setattr("sys.platform", "linux") + + from hermes_cli.voice import normalize_voice_record_key_for_prompt_toolkit + + assert normalize_voice_record_key_for_prompt_toolkit("alt+c") == "a-c" + assert normalize_voice_record_key_for_prompt_toolkit("alt+d") == "a-d" + assert normalize_voice_record_key_for_prompt_toolkit("alt+l") == "a-l" + + +class TestVoiceRecordKeyFromConfig: + """Round-11 Copilot review regression on #19835. + + ``load_config()`` preserves YAML scalar overrides, so a hand-edited + ``voice: true`` or ``voice: cmd+b`` made the naive + ``cfg.get('voice', {}).get('record_key')`` chain raise + AttributeError before voice could run. The shape-safe extractor + returns None for every malformed shape so the call-site fallback + (``normalize_…`` / ``format_…``) surfaces the documented default. + """ + + def test_dict_voice_with_string_record_key(self): + from hermes_cli.voice import voice_record_key_from_config + + assert voice_record_key_from_config({"voice": {"record_key": "ctrl+o"}}) == "ctrl+o" + + def test_non_dict_config_root(self): + from hermes_cli.voice import voice_record_key_from_config + + for bad_root in (None, True, 1, "ctrl+b", [], ["ctrl+b"]): + assert voice_record_key_from_config(bad_root) is None, bad_root + + def test_non_dict_voice_entry(self): + from hermes_cli.voice import voice_record_key_from_config + + for bad_voice in (None, True, "cmd+b", 42, ["ctrl+b"]): + assert voice_record_key_from_config({"voice": bad_voice}) is None, bad_voice + + def test_missing_record_key_returns_none(self): + from hermes_cli.voice import voice_record_key_from_config + + assert voice_record_key_from_config({"voice": {"beep_enabled": True}}) is None + assert voice_record_key_from_config({}) is None + + def test_normalizer_accepts_extractor_output_directly(self): + """voice_record_key_from_config + normalize_… must compose — + None / non-string scalars all fall back to c-b.""" + from hermes_cli.voice import ( + normalize_voice_record_key_for_prompt_toolkit, + voice_record_key_from_config, + ) + + for raw in (None, True, 1, "cmd+b", ["ctrl+b"]): + extracted = voice_record_key_from_config({"voice": raw}) + assert normalize_voice_record_key_for_prompt_toolkit(extracted) == "c-b" + + +class TestFormatVoiceRecordKeyForStatus: + """Round-10 Copilot review regression on #19835. + + ``/voice status`` used to print the raw scalar (``True`` / ``1``) + for non-string configs even though the actual binding falls back + to Ctrl+B. The formatter routes through the same normalizer so + status always matches what the CLI actually binds. + """ + + def test_ctrl_and_alt_letter_keys_render_canonically(self): + from hermes_cli.voice import format_voice_record_key_for_status + + assert format_voice_record_key_for_status("ctrl+b") == "Ctrl+B" + assert format_voice_record_key_for_status("ctrl+o") == "Ctrl+O" + assert format_voice_record_key_for_status("alt+r") == "Alt+R" + + def test_named_keys_render_in_title_case(self): + from hermes_cli.voice import format_voice_record_key_for_status + + assert format_voice_record_key_for_status("ctrl+space") == "Ctrl+Space" + assert format_voice_record_key_for_status("alt+enter") == "Alt+Enter" + assert format_voice_record_key_for_status("ctrl+esc") == "Ctrl+Escape" + + def test_aliases_render_via_normalized_form(self): + from hermes_cli.voice import format_voice_record_key_for_status + + assert format_voice_record_key_for_status("control+o") == "Ctrl+O" + assert format_voice_record_key_for_status("option+space") == "Alt+Space" + assert format_voice_record_key_for_status("opt+enter") == "Alt+Enter" + + def test_non_string_scalar_falls_back_to_ctrl_b_label(self): + from hermes_cli.voice import format_voice_record_key_for_status + + # Copilot round-10 regression: previously /voice status printed + # the raw scalar ("True" / "1") even though the actual binding + # fell back to Ctrl+B. + assert format_voice_record_key_for_status(True) == "Ctrl+B" + assert format_voice_record_key_for_status(1) == "Ctrl+B" + assert format_voice_record_key_for_status(None) == "Ctrl+B" + assert format_voice_record_key_for_status({}) == "Ctrl+B" + + def test_malformed_configs_fall_back_to_ctrl_b(self): + from hermes_cli.voice import format_voice_record_key_for_status + + assert format_voice_record_key_for_status("ctrl+spcae") == "Ctrl+B" + assert format_voice_record_key_for_status("ctrl+alt+r") == "Ctrl+B" + assert format_voice_record_key_for_status("") == "Ctrl+B" + assert format_voice_record_key_for_status(" ") == "Ctrl+B" + + class TestStopWithoutStart: def test_returns_none_when_no_recording_active(self, monkeypatch): """Idempotent no-op: stop before start must not raise or touch state.""" diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index 41b5194da6..469b8895ea 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -81,6 +81,173 @@ def test_dispatch_rejects_non_object_params(): } +def test_voice_toggle_returns_configured_record_key(monkeypatch): + monkeypatch.setattr( + server, + "_load_cfg", + lambda: {"voice": {"record_key": "ctrl+o"}}, + ) + monkeypatch.setitem( + sys.modules, + "tools.voice_mode", + types.SimpleNamespace( + check_voice_requirements=lambda: {"available": True, "details": ""} + ), + ) + # ``voice.toggle`` action=on mutates ``os.environ["HERMES_VOICE"]`` + # directly (CLI parity, runtime-only flag). Take monkeypatch + # ownership of the var so the change is reverted at teardown and + # later tests don't inherit a stale ON state (Copilot round-5 + # review on #19835). + monkeypatch.setenv("HERMES_VOICE", "0") + + on_resp = server.dispatch( + {"id": "voice-on", "method": "voice.toggle", "params": {"action": "on"}} + ) + status_resp = server.dispatch( + {"id": "voice-status", "method": "voice.toggle", "params": {"action": "status"}} + ) + + assert on_resp["result"]["record_key"] == "ctrl+o" + assert status_resp["result"]["record_key"] == "ctrl+o" + + +def test_voice_toggle_handles_non_dict_voice_cfg(monkeypatch): + """Round-3 Copilot review regression on #19835. + + ``_load_cfg()`` is raw ``yaml.safe_load()`` output — a hand-edited + ``voice: true`` / ``voice: cmd+b`` / ``voice: null`` leaves ``voice`` + as a bool/str/None, not a dict. Previously ``.get("record_key")`` + on a non-dict broke every ``voice.toggle`` branch. Now it falls + back to the documented default. + """ + monkeypatch.setitem( + sys.modules, + "tools.voice_mode", + types.SimpleNamespace( + check_voice_requirements=lambda: {"available": True, "details": ""} + ), + ) + + for bad in (True, "cmd+b", None, 42, ["ctrl+b"]): + monkeypatch.setattr(server, "_load_cfg", lambda b=bad: {"voice": b}) + + status_resp = server.dispatch( + {"id": "voice-status", "method": "voice.toggle", "params": {"action": "status"}} + ) + + assert status_resp["result"]["record_key"] == "ctrl+b", ( + f"voice.record_key fell back to default for voice={bad!r}" + ) + + # Round-4 follow-up: the YAML root itself may be a non-dict. A + # hand-edit that collapses config.yaml to a scalar / list would + # otherwise crash ``.get("voice")`` before the inner isinstance + # guard gets a chance to run. + for bad_root in (True, None, [], "ctrl+b", 42): + monkeypatch.setattr(server, "_load_cfg", lambda r=bad_root: r) + + status_resp = server.dispatch( + {"id": "voice-status-root", "method": "voice.toggle", "params": {"action": "status"}} + ) + + assert status_resp["result"]["record_key"] == "ctrl+b", ( + f"voice.record_key fell back to default for root={bad_root!r}" + ) + + +def test_voice_record_start_handles_non_dict_voice_cfg(monkeypatch): + """Round-7 Copilot review regression on #19835. + + The ``voice.record`` start path previously read + ``_load_cfg().get("voice", {}).get(...)`` without any shape checks. + When ``voice`` is a non-dict (bool/scalar/list) ``get`` raises + AttributeError and the handler returns 5025 instead of falling + back to the VAD defaults. Now it uses ``_voice_cfg_dict()`` and + non-numeric silence values are coerced to the documented defaults. + """ + captured: dict = {} + + def fake_start_continuous(**kwargs): + captured.update(kwargs) + + monkeypatch.setitem( + sys.modules, + "hermes_cli.voice", + types.SimpleNamespace(start_continuous=fake_start_continuous, stop_continuous=lambda: None), + ) + monkeypatch.setenv("HERMES_VOICE", "1") + + for bad in (True, "cmd+b", None, 42, ["ctrl+b"], {"silence_threshold": "loud"}): + captured.clear() + monkeypatch.setattr(server, "_load_cfg", lambda b=bad: {"voice": b}) + + resp = server.dispatch( + {"id": "voice-record", "method": "voice.record", "params": {"action": "start"}} + ) + + assert "result" in resp, f"voice.record raised for voice={bad!r}: {resp.get('error')}" + assert resp["result"]["status"] == "recording" + assert captured["silence_threshold"] == 200 + assert captured["silence_duration"] == 3.0 + + # Round-12 Copilot review regression on #19835: ``bool`` is a subclass + # of ``int``, so the naive ``isinstance(threshold, (int, float))`` + # guard would forward ``silence_threshold: true`` as ``1`` instead + # of falling back to the documented 200 default. + for bad_bool_cfg in ( + {"silence_threshold": True, "silence_duration": False}, + {"silence_threshold": False}, + {"silence_duration": True}, + ): + captured.clear() + monkeypatch.setattr(server, "_load_cfg", lambda c=bad_bool_cfg: {"voice": c}) + + resp = server.dispatch( + {"id": "voice-record-bool", "method": "voice.record", "params": {"action": "start"}} + ) + + assert "result" in resp, f"voice.record raised for bool cfg={bad_bool_cfg!r}" + assert captured["silence_threshold"] == 200, ( + f"bool silence_threshold leaked through for {bad_bool_cfg!r}" + ) + assert captured["silence_duration"] == 3.0, ( + f"bool silence_duration leaked through for {bad_bool_cfg!r}" + ) + + +def test_voice_toggle_tts_branch_also_carries_record_key(monkeypatch): + """Round-2 Copilot review regression on #19835. + + The ``tts`` branch used to omit ``record_key`` from its response, so a + TUI client would parse ``r.record_key ?? 'ctrl+b'`` and reset a + custom binding to the default on every TTS toggle. Every branch of + ``voice.toggle`` now carries the configured key so frontend state + stays authoritative. + """ + monkeypatch.setattr( + server, + "_load_cfg", + lambda: {"voice": {"record_key": "ctrl+space"}}, + ) + monkeypatch.setitem( + sys.modules, + "tools.voice_mode", + types.SimpleNamespace( + check_voice_requirements=lambda: {"available": True, "details": ""} + ), + ) + monkeypatch.setenv("HERMES_VOICE", "1") + monkeypatch.delenv("HERMES_VOICE_TTS", raising=False) + + tts_resp = server.dispatch( + {"id": "voice-tts", "method": "voice.toggle", "params": {"action": "tts"}} + ) + + assert tts_resp["result"]["record_key"] == "ctrl+space" + assert tts_resp["result"]["tts"] is True + + def test_load_enabled_toolsets_prefers_tui_env(monkeypatch): monkeypatch.setenv("HERMES_TUI_TOOLSETS", "web, terminal, ,memory") diff --git a/tui_gateway/server.py b/tui_gateway/server.py index 01407308da..2a4377c3f1 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -5278,6 +5278,30 @@ def _voice_tts_enabled() -> bool: return os.environ.get("HERMES_VOICE_TTS", "").strip() == "1" +def _voice_cfg_dict() -> dict: + """Shape-safe accessor for the ``voice:`` block in config.yaml. + + ``_load_cfg()`` returns raw ``yaml.safe_load()`` output, so both the + root AND ``voice`` may be any YAML scalar / list / None. A hand-edit + like ``voice: true`` or a malformed top-level config that parses to + a scalar would otherwise break ``.get("…")`` and take every + ``voice.*`` branch down with it (Copilot round-3..7 review on + #19835). Coerce through ``isinstance`` at every level so malformed + config falls back to an empty dict instead of crashing /voice. + """ + cfg = _load_cfg() + voice_cfg = cfg.get("voice") if isinstance(cfg, dict) else None + + return voice_cfg if isinstance(voice_cfg, dict) else {} + + +def _voice_record_key() -> str: + """Current ``voice.record_key`` value, documented default on error.""" + record_key = _voice_cfg_dict().get("record_key") + + return str(record_key) if isinstance(record_key, str) and record_key else "ctrl+b" + + @method("voice.toggle") def _(rid, params: dict) -> dict: """CLI parity for the ``/voice`` slash command. @@ -5298,8 +5322,13 @@ def _(rid, params: dict) -> dict: # Mirror CLI's _show_voice_status: include STT/TTS provider # availability so the user can tell at a glance *why* voice mode # isn't working ("STT provider: MISSING ..." is the common case). + # ``record_key`` mirrors the configured ``voice.record_key`` so the + # TUI can both bind it (frontend ``isVoiceToggleKey``) and display + # it in /voice status — previously the TUI hardcoded Ctrl+B and + # ignored the config (#18994). payload: dict = { "enabled": _voice_mode_enabled(), + "record_key": _voice_record_key(), "tts": _voice_tts_enabled(), } try: @@ -5336,7 +5365,14 @@ def _(rid, params: dict) -> dict: except Exception as e: logger.warning("voice: stop_continuous failed during toggle off: %s", e) - return _ok(rid, {"enabled": enabled, "tts": _voice_tts_enabled()}) + return _ok( + rid, + { + "enabled": enabled, + "record_key": _voice_record_key(), + "tts": _voice_tts_enabled(), + }, + ) if action == "tts": if not _voice_mode_enabled(): @@ -5344,7 +5380,18 @@ def _(rid, params: dict) -> dict: new_value = not _voice_tts_enabled() # Runtime-only flag (CLI parity) — see voice.toggle on/off above. os.environ["HERMES_VOICE_TTS"] = "1" if new_value else "0" - return _ok(rid, {"enabled": True, "tts": new_value}) + # Include ``record_key`` on every branch so a /voice tts toggle + # doesn't reset the TUI's cached shortcut to the default when a + # user has a custom binding configured (Copilot review, round 2 + # on #19835). Keeps parity with the status/on/off branches above. + return _ok( + rid, + { + "enabled": True, + "record_key": _voice_record_key(), + "tts": new_value, + }, + ) return _err(rid, 4013, f"unknown voice action: {action}") @@ -5376,15 +5423,26 @@ def _(rid, params: dict) -> dict: from hermes_cli.voice import start_continuous - voice_cfg = _load_cfg().get("voice", {}) + # Shape-safe lookups: malformed ``voice:`` YAML (bool/scalar/list) + # must not crash /voice with a 5025 — fall back to VAD defaults. + # + # Exclude ``bool`` from the numeric check since Python's bool is + # a subclass of int — a hand-edit like ``silence_threshold: true`` + # would otherwise forward as ``1`` instead of falling back to + # the documented 200 / 3.0 defaults (Copilot round-12 on #19835). + voice_cfg = _voice_cfg_dict() + threshold = voice_cfg.get("silence_threshold") + duration = voice_cfg.get("silence_duration") + safe_threshold = threshold if isinstance(threshold, (int, float)) and not isinstance(threshold, bool) else 200 + safe_duration = duration if isinstance(duration, (int, float)) and not isinstance(duration, bool) else 3.0 start_continuous( on_transcript=lambda t: _voice_emit("voice.transcript", {"text": t}), on_status=lambda s: _voice_emit("voice.status", {"state": s}), on_silent_limit=lambda: _voice_emit( "voice.transcript", {"no_speech_limit": True} ), - silence_threshold=voice_cfg.get("silence_threshold", 200), - silence_duration=voice_cfg.get("silence_duration", 3.0), + silence_threshold=safe_threshold, + silence_duration=safe_duration, ) return _ok(rid, {"status": "recording"}) diff --git a/ui-tui/src/__tests__/createSlashHandler.test.ts b/ui-tui/src/__tests__/createSlashHandler.test.ts index e8c50c05d2..c9447f16d8 100644 --- a/ui-tui/src/__tests__/createSlashHandler.test.ts +++ b/ui-tui/src/__tests__/createSlashHandler.test.ts @@ -173,6 +173,64 @@ describe('createSlashHandler', () => { expect(ctx.transcript.sys).toHaveBeenCalledWith(expect.stringContaining('usage: /skills')) }) + // Regressions from Copilot review on #19835: /voice output + frontend + // binding state must both track the gateway's fresh ``record_key`` on + // every response, or a config edit shows the new shortcut in text + // while push-to-talk still fires the old one until the next mtime + // poll (~5s). + it('/voice status renders the gateway record_key and pushes it into frontend state', async () => { + const rpc = vi.fn(() => Promise.resolve({ enabled: true, record_key: 'ctrl+space', tts: false })) + const ctx = buildCtx({ gateway: { ...buildGateway(), rpc } }) + + expect(createSlashHandler(ctx)('/voice status')).toBe(true) + await vi.waitFor(() => { + expect(ctx.transcript.sys).toHaveBeenCalledWith(' Record key: Ctrl+Space') + }) + expect(ctx.voice.setVoiceRecordKey).toHaveBeenCalledWith( + expect.objectContaining({ ch: 'space', mod: 'ctrl', named: 'space' }) + ) + }) + + it('/voice on renders the configured binding for the start/stop hint', async () => { + const rpc = vi.fn(() => Promise.resolve({ enabled: true, record_key: 'alt+r', tts: false })) + const ctx = buildCtx({ gateway: { ...buildGateway(), rpc } }) + + expect(createSlashHandler(ctx)('/voice on')).toBe(true) + await vi.waitFor(() => { + expect(ctx.transcript.sys).toHaveBeenCalledWith('Voice mode enabled') + expect(ctx.transcript.sys).toHaveBeenCalledWith(' Alt+R to start/stop recording') + }) + expect(ctx.voice.setVoiceRecordKey).toHaveBeenCalledWith( + expect.objectContaining({ ch: 'r', mod: 'alt' }) + ) + }) + + it('/voice falls back to Ctrl+B when the gateway response omits record_key', async () => { + const rpc = vi.fn(() => Promise.resolve({ enabled: false, tts: false })) + const ctx = buildCtx({ gateway: { ...buildGateway(), rpc } }) + + expect(createSlashHandler(ctx)('/voice status')).toBe(true) + await vi.waitFor(() => { + expect(ctx.transcript.sys).toHaveBeenCalledWith(' Record key: Ctrl+B') + }) + }) + + // Round-2 Copilot review on #19835: a response missing ``record_key`` + // (e.g. the old tts branch, or any future branch that forgets to + // include it) MUST NOT clobber the user's cached binding back to + // Ctrl+B. The label still renders the default for display; the + // frontend state keeps whatever was last authoritatively set. + it('/voice tts without record_key does not clobber cached frontend binding', async () => { + const rpc = vi.fn(() => Promise.resolve({ enabled: true, tts: true })) + const ctx = buildCtx({ gateway: { ...buildGateway(), rpc } }) + + expect(createSlashHandler(ctx)('/voice tts')).toBe(true) + await vi.waitFor(() => { + expect(ctx.transcript.sys).toHaveBeenCalledWith('Voice TTS enabled.') + }) + expect(ctx.voice.setVoiceRecordKey).not.toHaveBeenCalled() + }) + it('cycles details mode and persists it', async () => { const ctx = buildCtx() @@ -648,7 +706,8 @@ const buildTranscript = () => ({ }) const buildVoice = () => ({ - setVoiceEnabled: vi.fn() + setVoiceEnabled: vi.fn(), + setVoiceRecordKey: vi.fn() }) interface Ctx { diff --git a/ui-tui/src/__tests__/platform.test.ts b/ui-tui/src/__tests__/platform.test.ts index 4166f0b71f..77f1347a3a 100644 --- a/ui-tui/src/__tests__/platform.test.ts +++ b/ui-tui/src/__tests__/platform.test.ts @@ -67,11 +67,15 @@ describe('isVoiceToggleKey', () => { expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'B')).toBe(true) }) - it('matches Cmd+B on macOS (preserve platform muscle memory)', async () => { + it('matches kitty-style Cmd+B on macOS via key.super', async () => { const { isVoiceToggleKey } = await importPlatform('darwin') - expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b')).toBe(true) expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b')).toBe(true) + // ``key.meta`` is NOT accepted as Cmd — hermes-ink uses meta for + // Alt too, so accepting it leaked Alt+B into the default binding + // (Copilot round-6 review on #19835). Legacy-terminal mac users + // get strict Ctrl+B. + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b')).toBe(false) }) it('matches Ctrl+B on non-macOS platforms', async () => { @@ -89,6 +93,449 @@ describe('isVoiceToggleKey', () => { }) }) +describe('parseVoiceRecordKey (#18994)', () => { + it('falls back to Ctrl+B for empty input', async () => { + const { DEFAULT_VOICE_RECORD_KEY, parseVoiceRecordKey } = await importPlatform('linux') + + expect(parseVoiceRecordKey('')).toEqual(DEFAULT_VOICE_RECORD_KEY) + }) + + it('parses ctrl+ bindings', async () => { + const { parseVoiceRecordKey } = await importPlatform('linux') + + expect(parseVoiceRecordKey('ctrl+o')).toEqual({ ch: 'o', mod: 'ctrl', raw: 'ctrl+o' }) + expect(parseVoiceRecordKey('Ctrl+R')).toEqual({ ch: 'r', mod: 'ctrl', raw: 'ctrl+r' }) + }) + + it('parses alt/super aliases', async () => { + const { parseVoiceRecordKey } = await importPlatform('linux') + + expect(parseVoiceRecordKey('alt+b').mod).toBe('alt') + expect(parseVoiceRecordKey('option+b').mod).toBe('alt') + expect(parseVoiceRecordKey('super+b').mod).toBe('super') + expect(parseVoiceRecordKey('win+b').mod).toBe('super') + }) + + it('treats ambiguous mac modifiers (meta / cmd / command) as unrecognised', async () => { + const { DEFAULT_VOICE_RECORD_KEY, parseVoiceRecordKey } = await importPlatform('linux') + + // ``meta`` / ``cmd`` / ``command`` are ambiguous on the wire: + // hermes-ink sets ``key.meta`` for plain Alt on every platform AND + // for Cmd on legacy macOS terminals. Accepting any of them would + // produce a display/binding mismatch (Copilot round-6 review on + // #19835). Users on modern kitty-style terminals spell the + // platform action modifier ``super`` / ``win``. + expect(parseVoiceRecordKey('meta+b')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('cmd+b')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('command+b')).toEqual(DEFAULT_VOICE_RECORD_KEY) + }) + + it('parses named keys (space, enter, tab, escape, backspace, delete)', async () => { + const { parseVoiceRecordKey } = await importPlatform('linux') + + // Every named token from the CLI's prompt_toolkit ``c-`` set is + // accepted with both the canonical name and its common alias. + expect(parseVoiceRecordKey('ctrl+space')).toEqual({ + ch: 'space', + mod: 'ctrl', + named: 'space', + raw: 'ctrl+space' + }) + expect(parseVoiceRecordKey('alt+enter').named).toBe('enter') + expect(parseVoiceRecordKey('alt+return').named).toBe('enter') // ``return`` ↔ ``enter`` + expect(parseVoiceRecordKey('ctrl+tab').named).toBe('tab') + expect(parseVoiceRecordKey('ctrl+escape').named).toBe('escape') + expect(parseVoiceRecordKey('ctrl+esc').named).toBe('escape') // ``esc`` alias + expect(parseVoiceRecordKey('ctrl+backspace').named).toBe('backspace') + expect(parseVoiceRecordKey('ctrl+delete').named).toBe('delete') + expect(parseVoiceRecordKey('ctrl+del').named).toBe('delete') // ``del`` alias + }) + + it('falls back to Ctrl+B for unrecognised multi-character tokens', async () => { + const { DEFAULT_VOICE_RECORD_KEY, parseVoiceRecordKey } = await importPlatform('linux') + + // Typos / unsupported names (``ctrl+spcae``, ``ctrl+f5``, …) fall back + // to the documented Ctrl+B default rather than silently disabling the + // binding. + expect(parseVoiceRecordKey('ctrl+spcae')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('ctrl+f5')).toEqual(DEFAULT_VOICE_RECORD_KEY) + }) + + // Round-3 Copilot review regressions on #19835. + it('does not throw on non-string YAML scalars — falls back instead', async () => { + const { DEFAULT_VOICE_RECORD_KEY, parseVoiceRecordKey } = await importPlatform('linux') + + // ``config.get full`` surfaces raw YAML values; ``voice.record_key: 1`` + // or ``voice.record_key: true`` would otherwise crash ``.trim()``. + expect(parseVoiceRecordKey(1 as unknown as string)).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey(true as unknown as string)).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey(null as unknown as string)).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey(undefined as unknown as string)).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey({} as unknown as string)).toEqual(DEFAULT_VOICE_RECORD_KEY) + }) + + it('rejects multi-modifier chords rather than silently dropping extras', async () => { + const { DEFAULT_VOICE_RECORD_KEY, parseVoiceRecordKey } = await importPlatform('linux') + + // Previously ``ctrl+alt+r`` parsed as ``ctrl+r`` and ``cmd+ctrl+b`` as + // ``super+b`` — a typo silently bound a different shortcut. Now a + // multi-modifier spelling falls back to the documented default. + expect(parseVoiceRecordKey('ctrl+alt+r')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('cmd+ctrl+b')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('alt+ctrl+space')).toEqual(DEFAULT_VOICE_RECORD_KEY) + }) + + // Round-4 Copilot review regressions on #19835. + it('rejects bare-char configs without an explicit modifier', async () => { + const { DEFAULT_VOICE_RECORD_KEY, parseVoiceRecordKey } = await importPlatform('linux') + + // The classic CLI's prompt_toolkit binds raw-char configs to the key + // itself (``c-o`` requires an explicit modifier); rewriting ``o`` + // → ``ctrl+o`` would silently diverge the two runtimes. Refuse. + expect(parseVoiceRecordKey('o')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('b')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('space')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('escape')).toEqual(DEFAULT_VOICE_RECORD_KEY) + }) + + it('rejects ctrl+c / ctrl+d / ctrl+l — reserved by the TUI input handler', async () => { + const { DEFAULT_VOICE_RECORD_KEY, parseVoiceRecordKey } = await importPlatform('linux') + + // ``useInputHandlers()`` intercepts these before the voice check, + // so a binding like ``ctrl+c`` would be advertised but never fire. + // Fall back to the documented default instead of lying to the user. + expect(parseVoiceRecordKey('ctrl+c')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('ctrl+d')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('ctrl+l')).toEqual(DEFAULT_VOICE_RECORD_KEY) + // Alt-modifier versions of those letters are NOT intercepted, so + // they remain usable. + expect(parseVoiceRecordKey('alt+c').mod).toBe('alt') + // ``ctrl+x`` is intentionally allowed — only intercepted during + // queue-edit (``queueEditIdx !== null``), so the voice binding + // works for most of the session (Copilot round-8 review). + expect(parseVoiceRecordKey('ctrl+x').mod).toBe('ctrl') + expect(parseVoiceRecordKey('ctrl+x').ch).toBe('x') + }) + + it('rejects super+{c,d,l,v} on macOS — action-mod chords are claimed before voice', async () => { + const { DEFAULT_VOICE_RECORD_KEY, parseVoiceRecordKey } = await importPlatform('darwin') + + // On macOS super+c/d/l/v are copy / exit / clear / paste. Reject at + // parse time so /voice status doesn't advertise dead bindings. + expect(parseVoiceRecordKey('super+c')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('super+d')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('super+l')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('super+v')).toEqual(DEFAULT_VOICE_RECORD_KEY) + // Other super letters still work (no global chord claims them). + expect(parseVoiceRecordKey('super+b').mod).toBe('super') + expect(parseVoiceRecordKey('super+o').mod).toBe('super') + }) + + it('allows super+{c,d,l,v} on Linux/Windows — those globals key off Ctrl, not Super', async () => { + const { parseVoiceRecordKey } = await importPlatform('linux') + + // Kitty/CSI-u users on non-mac report Cmd/Super as ``key.super``, + // but the TUI's global shortcuts (copy/exit/clear/paste) key off + // Ctrl there, so ``super+`` doesn't collide. Reject would + // silently coerce valid configs to Ctrl+B (Copilot round-8 review). + expect(parseVoiceRecordKey('super+c').mod).toBe('super') + expect(parseVoiceRecordKey('super+d').mod).toBe('super') + expect(parseVoiceRecordKey('super+l').mod).toBe('super') + expect(parseVoiceRecordKey('super+v').mod).toBe('super') + }) + + it('rejects alt+{c,d,l} on macOS — meta-as-alt collides with isAction', async () => { + const { DEFAULT_VOICE_RECORD_KEY, parseVoiceRecordKey } = await importPlatform('darwin') + + // hermes-ink reports Alt as ``key.meta`` on many terminals, and + // ``isActionMod`` on darwin accepts ``key.meta`` as the action + // modifier. So ``alt+c`` / ``alt+d`` / ``alt+l`` get claimed by + // isCopyShortcut / isAction('d') / isAction('l') before voice + // runs (Copilot round-12 on #19835). + expect(parseVoiceRecordKey('alt+c')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('alt+d')).toEqual(DEFAULT_VOICE_RECORD_KEY) + expect(parseVoiceRecordKey('alt+l')).toEqual(DEFAULT_VOICE_RECORD_KEY) + // Other alt letters stay usable on darwin. + expect(parseVoiceRecordKey('alt+r').mod).toBe('alt') + expect(parseVoiceRecordKey('alt+space').mod).toBe('alt') + }) + + it('allows alt+{c,d,l} on Linux/Windows — non-mac isAction keys off Ctrl', async () => { + const { parseVoiceRecordKey } = await importPlatform('linux') + + // On Linux/Windows ``isActionMod`` ignores key.meta, so alt+ + // doesn't collide with copy/exit/clear. Those configs stay usable. + expect(parseVoiceRecordKey('alt+c').mod).toBe('alt') + expect(parseVoiceRecordKey('alt+d').mod).toBe('alt') + expect(parseVoiceRecordKey('alt+l').mod).toBe('alt') + }) + + // Round-5 Copilot review regressions on #19835. + it('super+ does NOT fire on key.meta-only events (Alt+X false-fire guard)', async () => { + const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin') + + // hermes-ink sets ``key.meta`` for Alt/Option AND for bare Esc on + // some macOS terminals. The super branch used to accept + // ``isMac && key.meta`` as a Cmd fallback, which made super+ + // bindings silently fire on Alt+ / bare Esc. + const superB = parseVoiceRecordKey('super+b') + const superSpace = parseVoiceRecordKey('super+space') + const superEscape = parseVoiceRecordKey('super+escape') + + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', superB)).toBe(false) + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, ' ', superSpace)).toBe(false) + expect(isVoiceToggleKey({ ctrl: false, escape: true, meta: true, super: false }, '', superEscape)).toBe(false) + }) + + // Round-6 Copilot review regressions on #19835. + it('default ctrl+b does NOT fire on Alt+B via isActionMod meta leak', async () => { + const { DEFAULT_VOICE_RECORD_KEY, isVoiceToggleKey } = await importPlatform('darwin') + + // ``isActionMod(key)`` on darwin was accepting ``key.meta`` as the + // action modifier, so Alt+B (key.meta=true) fired the default + // ctrl+b binding. Now the Cmd-fallback path requires literal + // ``key.super`` on macOS and rejects ``key.meta``. + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', DEFAULT_VOICE_RECORD_KEY)).toBe(false) + // Literal Ctrl+B and Cmd+B (kitty-style) still work on darwin. + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'b', DEFAULT_VOICE_RECORD_KEY)).toBe(true) + expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', DEFAULT_VOICE_RECORD_KEY)).toBe(true) + }) + + it('ctrl+ rejects chords with extra alt / meta / super bits', async () => { + const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('linux') + const ctrlO = parseVoiceRecordKey('ctrl+o') + + // ``ctrl+o`` must fire ONLY on literal Ctrl+O, not on + // Ctrl+Alt+O / Ctrl+Cmd+O / Ctrl+Meta+O — otherwise the runtime + // matches a different chord than the parser would let you + // configure. + expect(isVoiceToggleKey({ alt: true, ctrl: true, meta: false, super: false }, 'o', ctrlO)).toBe(false) + expect(isVoiceToggleKey({ ctrl: true, meta: true, super: false }, 'o', ctrlO)).toBe(false) + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: true }, 'o', ctrlO)).toBe(false) + // Sanity: plain Ctrl+O still fires. + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'o', ctrlO)).toBe(true) + }) + + it('super+ rejects chords with extra ctrl / alt / meta bits', async () => { + const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('linux') + const superB = parseVoiceRecordKey('super+b') + + expect(isVoiceToggleKey({ alt: true, ctrl: false, meta: false, super: true }, 'b', superB)).toBe(false) + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: true }, 'b', superB)).toBe(false) + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: true }, 'b', superB)).toBe(false) + // Sanity: plain Super+B still fires. + expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', superB)).toBe(true) + }) + + it('alt+escape does not fire on bare Esc meta-shape', async () => { + const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin') + const altEscape = parseVoiceRecordKey('alt+escape') + + // Some terminals surface bare Esc as meta=true + escape=true. + expect(isVoiceToggleKey({ ctrl: false, escape: true, meta: true, super: false }, '', altEscape)).toBe(false) + // Explicit alt bit (kitty-style) still fires the configured chord. + expect(isVoiceToggleKey({ alt: true, ctrl: false, escape: true, meta: false, super: false }, '', altEscape)).toBe(true) + }) + + it('rejects matches when Shift is held (different chord than configured)', async () => { + const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('linux') + + // Parser rejects multi-modifier configs like ``ctrl+shift+tab``, + // so the runtime matcher must also reject Shift-held events — + // otherwise ``ctrl+tab`` would fire on Ctrl+Shift+Tab. + const ctrlTab = parseVoiceRecordKey('ctrl+tab') + const altEnter = parseVoiceRecordKey('alt+enter') + const ctrlO = parseVoiceRecordKey('ctrl+o') + + expect(isVoiceToggleKey({ ctrl: true, meta: false, shift: true, super: false, tab: true }, '', ctrlTab)).toBe(false) + expect(isVoiceToggleKey({ alt: true, ctrl: false, meta: false, return: true, shift: true, super: false }, '', altEnter)).toBe(false) + expect(isVoiceToggleKey({ ctrl: true, meta: false, shift: true, super: false }, 'o', ctrlO)).toBe(false) + + // Sanity: same events without Shift still fire. + expect(isVoiceToggleKey({ ctrl: true, meta: false, shift: false, super: false, tab: true }, '', ctrlTab)).toBe(true) + expect(isVoiceToggleKey({ ctrl: true, meta: false, shift: false, super: false }, 'o', ctrlO)).toBe(true) + }) +}) + +describe('formatVoiceRecordKey (#18994)', () => { + it('renders as the user expects in /voice status', async () => { + const { formatVoiceRecordKey, parseVoiceRecordKey } = await importPlatform('linux') + + expect(formatVoiceRecordKey(parseVoiceRecordKey('ctrl+b'))).toBe('Ctrl+B') + expect(formatVoiceRecordKey(parseVoiceRecordKey('ctrl+o'))).toBe('Ctrl+O') + expect(formatVoiceRecordKey(parseVoiceRecordKey('alt+r'))).toBe('Alt+R') + // ``super``/``win`` render as ``Super`` on non-mac so the hint + // doesn't tell Linux/Windows users to press a Cmd key they don't + // have. + expect(formatVoiceRecordKey(parseVoiceRecordKey('super+b'))).toBe('Super+B') + }) + + it('renders named keys in title case (Ctrl+Space, Ctrl+Enter)', async () => { + const { formatVoiceRecordKey, parseVoiceRecordKey } = await importPlatform('linux') + + expect(formatVoiceRecordKey(parseVoiceRecordKey('ctrl+space'))).toBe('Ctrl+Space') + expect(formatVoiceRecordKey(parseVoiceRecordKey('alt+enter'))).toBe('Alt+Enter') + expect(formatVoiceRecordKey(parseVoiceRecordKey('ctrl+esc'))).toBe('Ctrl+Escape') + expect(formatVoiceRecordKey(parseVoiceRecordKey('super+space'))).toBe('Super+Space') + }) +}) + +describe('isVoiceToggleKey honours configured record key (#18994)', () => { + it('binds the configured letter, not hardcoded b', async () => { + const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('linux') + const ctrlO = parseVoiceRecordKey('ctrl+o') + + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'o', ctrlO)).toBe(true) + // The old hardcoded 'b' must NOT match when the user configured 'o'. + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'b', ctrlO)).toBe(false) + }) + + it('alt+ binding matches alt OR meta (terminal-protocol parity)', async () => { + const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('linux') + const altR = parseVoiceRecordKey('alt+r') + + expect(isVoiceToggleKey({ alt: true, ctrl: false, meta: false, super: false }, 'r', altR)).toBe(true) + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'r', altR)).toBe(true) + expect(isVoiceToggleKey({ ctrl: false, meta: false, super: false }, 'r', altR)).toBe(false) + }) + + it('binds named keys via ink event flags (space → ch === " ", enter → key.return, …)', async () => { + const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('linux') + + const ctrlSpace = parseVoiceRecordKey('ctrl+space') + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, ' ', ctrlSpace)).toBe(true) + // Single-char ``b`` must NOT match a ``space``-configured binding. + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'b', ctrlSpace)).toBe(false) + // Space without the configured modifier must not fire either. + expect(isVoiceToggleKey({ ctrl: false, meta: false, super: false }, ' ', ctrlSpace)).toBe(false) + + const ctrlEnter = parseVoiceRecordKey('ctrl+enter') + expect(isVoiceToggleKey({ ctrl: true, meta: false, return: true, super: false }, '', ctrlEnter)).toBe(true) + expect(isVoiceToggleKey({ ctrl: true, meta: false, return: false, super: false }, '', ctrlEnter)).toBe(false) + + const altTab = parseVoiceRecordKey('alt+tab') + expect(isVoiceToggleKey({ alt: true, ctrl: false, meta: false, super: false, tab: true }, '', altTab)).toBe(true) + expect(isVoiceToggleKey({ alt: false, ctrl: false, meta: false, super: false, tab: true }, '', altTab)).toBe(false) + + const ctrlEscape = parseVoiceRecordKey('ctrl+escape') + expect(isVoiceToggleKey({ ctrl: true, escape: true, meta: false, super: false }, '', ctrlEscape)).toBe(true) + expect(isVoiceToggleKey({ ctrl: true, escape: false, meta: false, super: false }, '', ctrlEscape)).toBe(false) + + const ctrlBackspace = parseVoiceRecordKey('ctrl+backspace') + expect(isVoiceToggleKey({ backspace: true, ctrl: true, meta: false, super: false }, '', ctrlBackspace)).toBe(true) + + const ctrlDelete = parseVoiceRecordKey('ctrl+delete') + expect(isVoiceToggleKey({ ctrl: true, delete: true, meta: false, super: false }, '', ctrlDelete)).toBe(true) + }) + + it('omitted configured key falls back to ctrl+b (back-compat)', async () => { + const { isVoiceToggleKey } = await importPlatform('linux') + + // No third arg → DEFAULT_VOICE_RECORD_KEY → Ctrl+B behaviour. + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'b')).toBe(true) + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'o')).toBe(false) + }) + + // Regressions from Copilot review on #19835: the previous implementation + // accepted ``isActionMod(key)`` in the ``ctrl`` branch for every + // configured key, so bare Esc (which hermes-ink reports with + // ``key.meta`` on some macOS terminals) fired ``ctrl+escape``, and + // Alt+Space / Alt+Tab fired ``ctrl+space`` / ``ctrl+tab``. The fallback + // is now gated to the documented default (``ctrl+b``) only. + it('ctrl+escape does NOT fire on bare Esc via key.meta on macOS', async () => { + const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin') + const ctrlEscape = parseVoiceRecordKey('ctrl+escape') + + // Bare Esc on a legacy macOS terminal: ``key.meta: true``, ``key.escape: true``, no ctrl. + expect(isVoiceToggleKey({ ctrl: false, escape: true, meta: true, super: false }, '', ctrlEscape)).toBe(false) + // Real Ctrl+Esc still fires. + expect(isVoiceToggleKey({ ctrl: true, escape: true, meta: false, super: false }, '', ctrlEscape)).toBe(true) + }) + + it('ctrl+space does NOT fire on Alt+Space on macOS', async () => { + const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin') + const ctrlSpace = parseVoiceRecordKey('ctrl+space') + + // Alt+Space surfaces as ``key.meta: true`` with space char. + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, ' ', ctrlSpace)).toBe(false) + // Real Ctrl+Space still fires. + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, ' ', ctrlSpace)).toBe(true) + }) + + it('default ctrl+b accepts raw Ctrl+B and kitty-style Cmd+B on macOS', async () => { + const { DEFAULT_VOICE_RECORD_KEY, isVoiceToggleKey } = await importPlatform('darwin') + + // Raw Ctrl+B: always works. + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'b', DEFAULT_VOICE_RECORD_KEY)).toBe(true) + // Cmd+B via kitty-style ``key.super``: still works. + expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', DEFAULT_VOICE_RECORD_KEY)).toBe(true) + // Cmd+B via legacy ``key.meta`` NO LONGER works — ``key.meta`` is + // hermes-ink's Alt signal, so accepting it leaked Alt+B into the + // default binding (Copilot round-6 review on #19835). + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', DEFAULT_VOICE_RECORD_KEY)).toBe(false) + }) + + it('custom ctrl+ does NOT accept Cmd fallback on macOS', async () => { + const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin') + const ctrlO = parseVoiceRecordKey('ctrl+o') + + // Only ``ctrl+b`` gets the action-modifier fallback; ``ctrl+o`` must + // be a literal Ctrl bit — otherwise Cmd+O would steal the shortcut. + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'o', ctrlO)).toBe(false) + expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'o', ctrlO)).toBe(false) + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'o', ctrlO)).toBe(true) + }) + + it('super+b renders "Cmd+B" on darwin and requires the literal key.super bit', async () => { + const { formatVoiceRecordKey, isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin') + const superB = parseVoiceRecordKey('super+b') + + expect(formatVoiceRecordKey(superB)).toBe('Cmd+B') + // Kitty-style: key.super fires the binding. + expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', superB)).toBe(true) + // ``key.meta`` is NOT accepted — hermes-ink uses meta for Alt too, + // so accepting it here would make super+b silently fire on Alt+B + // (Copilot round-5 review on #19835). + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', superB)).toBe(false) + // Ctrl held at the same time → reject (different chord). + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: true }, 'b', superB)).toBe(false) + }) + + // Round-2 Copilot review regressions on #19835. + it('super+b renders "Super+B" on Linux (not "Cmd+B")', async () => { + const { formatVoiceRecordKey, parseVoiceRecordKey } = await importPlatform('linux') + + expect(formatVoiceRecordKey(parseVoiceRecordKey('super+b'))).toBe('Super+B') + expect(formatVoiceRecordKey(parseVoiceRecordKey('win+b'))).toBe('Super+B') + }) + + it('super+b still renders "Cmd+B" on macOS', async () => { + const { formatVoiceRecordKey, parseVoiceRecordKey } = await importPlatform('darwin') + + expect(formatVoiceRecordKey(parseVoiceRecordKey('super+b'))).toBe('Cmd+B') + expect(formatVoiceRecordKey(parseVoiceRecordKey('win+b'))).toBe('Cmd+B') + }) + + it('ctrl+b aliases (control+b, "ctrl + b") still accept Cmd+B fallback on macOS', async () => { + const { isVoiceToggleKey, parseVoiceRecordKey } = await importPlatform('darwin') + const controlB = parseVoiceRecordKey('control+b') + const spacedB = parseVoiceRecordKey('ctrl + b') + + // Both parse to the documented default semantically; both must keep + // the macOS Cmd+B muscle-memory fallback via kitty-style key.super. + // ``key.meta`` is NOT accepted — that's hermes-ink's Alt signal + // (round-6 review), so legacy-terminal users get strict Ctrl+B. + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', controlB)).toBe(false) + expect(isVoiceToggleKey({ ctrl: false, meta: true, super: false }, 'b', spacedB)).toBe(false) + expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', controlB)).toBe(true) + expect(isVoiceToggleKey({ ctrl: false, meta: false, super: true }, 'b', spacedB)).toBe(true) + // Literal Ctrl+B still fires. + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'b', controlB)).toBe(true) + // And still reject a ctrl bit on a different letter. + expect(isVoiceToggleKey({ ctrl: true, meta: false, super: false }, 'o', controlB)).toBe(false) + }) +}) + describe('isMacActionFallback', () => { it('routes raw Ctrl+K and Ctrl+W to readline kill-to-end / delete-word on macOS', async () => { const { isMacActionFallback } = await importPlatform('darwin') diff --git a/ui-tui/src/__tests__/textInputPassThrough.test.ts b/ui-tui/src/__tests__/textInputPassThrough.test.ts new file mode 100644 index 0000000000..5988580f9b --- /dev/null +++ b/ui-tui/src/__tests__/textInputPassThrough.test.ts @@ -0,0 +1,43 @@ +import { describe, expect, it } from 'vitest' + +import { shouldPassThroughToGlobalHandler } from '../components/textInput.js' +import { DEFAULT_VOICE_RECORD_KEY, parseVoiceRecordKey } from '../lib/platform.js' + +const key = (overrides: Record = {}) => + ({ ctrl: false, meta: false, ...overrides }) as any + +describe('shouldPassThroughToGlobalHandler', () => { + it('passes through the configured voice shortcut while composer is focused', () => { + expect( + shouldPassThroughToGlobalHandler('o', key({ ctrl: true }), parseVoiceRecordKey('ctrl+o')) + ).toBe(true) + expect( + shouldPassThroughToGlobalHandler('r', key({ meta: true }), parseVoiceRecordKey('alt+r')) + ).toBe(true) + expect( + shouldPassThroughToGlobalHandler(' ', key({ ctrl: true }), parseVoiceRecordKey('ctrl+space')) + ).toBe(true) + expect( + shouldPassThroughToGlobalHandler('', key({ ctrl: true, return: true }), parseVoiceRecordKey('ctrl+enter')) + ).toBe(true) + }) + + it('keeps the legacy default pass-through when no custom key is provided', () => { + expect(shouldPassThroughToGlobalHandler('b', key({ ctrl: true }), DEFAULT_VOICE_RECORD_KEY)).toBe(true) + expect(shouldPassThroughToGlobalHandler('b', key({ ctrl: true }))).toBe(true) + }) + + it('does not swallow ordinary typing keys', () => { + expect(shouldPassThroughToGlobalHandler('h', key(), parseVoiceRecordKey('ctrl+o'))).toBe(false) + expect(shouldPassThroughToGlobalHandler('o', key(), parseVoiceRecordKey('ctrl+o'))).toBe(false) + }) + + it('always passes through non-voice global control keys', () => { + expect(shouldPassThroughToGlobalHandler('c', key({ ctrl: true }))).toBe(true) + expect(shouldPassThroughToGlobalHandler('x', key({ ctrl: true }))).toBe(true) + expect(shouldPassThroughToGlobalHandler('', key({ escape: true }))).toBe(true) + expect(shouldPassThroughToGlobalHandler('', key({ tab: true }))).toBe(true) + expect(shouldPassThroughToGlobalHandler('', key({ pageUp: true }))).toBe(true) + expect(shouldPassThroughToGlobalHandler('', key({ pageDown: true }))).toBe(true) + }) +}) diff --git a/ui-tui/src/__tests__/useConfigSync.test.ts b/ui-tui/src/__tests__/useConfigSync.test.ts index fc2dad19f1..39020d2763 100644 --- a/ui-tui/src/__tests__/useConfigSync.test.ts +++ b/ui-tui/src/__tests__/useConfigSync.test.ts @@ -1,13 +1,15 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { $uiState, resetUiState } from '../app/uiStore.js' import { applyDisplay, + hydrateFullConfig, normalizeBusyInputMode, normalizeIndicatorStyle, normalizeMouseTracking, normalizeStatusBar } from '../app/useConfigSync.js' +import type { ParsedVoiceRecordKey } from '../lib/platform.js' describe('applyDisplay', () => { beforeEach(() => { @@ -292,3 +294,139 @@ describe('applyDisplay → tui_status_indicator', () => { expect($uiState.get().indicatorStyle).toBe('kaomoji') }) }) + +// Regressions from Copilot review on #19835: the config-hydration path +// for voice.record_key was untested, so a future regression in the +// hydration or mtime-reapply wiring would slip past the suite. +describe('applyDisplay → voice.record_key (#18994)', () => { + beforeEach(() => { + resetUiState() + }) + + it('parses voice.record_key and pushes it through the setter', () => { + const setBell = vi.fn() + const setVoiceRecordKey = vi.fn() + + applyDisplay( + { config: { display: {}, voice: { record_key: 'ctrl+space' } } }, + setBell, + setVoiceRecordKey + ) + + expect(setVoiceRecordKey).toHaveBeenCalledWith( + expect.objectContaining({ ch: 'space', mod: 'ctrl', named: 'space', raw: 'ctrl+space' }) + ) + }) + + it('falls back to the documented default when voice.record_key is missing', () => { + const setBell = vi.fn() + const setVoiceRecordKey = vi.fn() + + applyDisplay({ config: { display: {} } }, setBell, setVoiceRecordKey) + + expect(setVoiceRecordKey).toHaveBeenCalledWith( + expect.objectContaining({ ch: 'b', mod: 'ctrl', raw: 'ctrl+b' }) + ) + }) + + it('is a no-op when the voice setter is not passed (back-compat)', () => { + const setBell = vi.fn() + + // applyDisplay is used in the setVoiceEnabled-less init path too; + // omitting the third arg must not throw. + expect(() => + applyDisplay({ config: { display: {}, voice: { record_key: 'alt+r' } } }, setBell) + ).not.toThrow() + }) + + it('does not reset voiceRecordKey when cfg is null (transient RPC failure)', () => { + const setBell = vi.fn() + const setVoiceRecordKey = vi.fn() + + // quietRpc() collapses request failures to null. Resetting the + // cached shortcut on every null would clobber a custom binding + // after one transient error until the next successful poll + // (Copilot round-8 review on #19835). + applyDisplay(null, setBell, setVoiceRecordKey) + + expect(setVoiceRecordKey).not.toHaveBeenCalled() + // bell is still applied (defaults to false on null), so the setter + // runs — we specifically only skip voiceRecordKey. + expect(setBell).toHaveBeenCalledWith(false) + }) +}) + +// Round-12 Copilot review regression on #19835: the live mtime-reload +// path was previously untested, so a regression in the polling/RPC +// wiring to applyDisplay would only be visible at runtime. The fetch +// + apply body is now shared as ``hydrateFullConfig()``, exercised +// directly from both the initial hydration and the poll-tick body. +describe('hydrateFullConfig', () => { + beforeEach(() => { + resetUiState() + }) + + const makeFakeGw = (payload: unknown) => + ({ + request: vi.fn(() => Promise.resolve(payload)), + on: vi.fn(), + off: vi.fn() + }) as any + + it('re-applies voice.record_key from a fresh config.get full response', async () => { + const gw = makeFakeGw({ config: { display: {}, voice: { record_key: 'ctrl+o' } } }) + const setBell = vi.fn() + const setVoiceRecordKey = vi.fn() + + await hydrateFullConfig(gw, setBell, setVoiceRecordKey) + + expect(gw.request).toHaveBeenCalledWith('config.get', { key: 'full' }) + expect(setVoiceRecordKey).toHaveBeenCalledWith( + expect.objectContaining({ ch: 'o', mod: 'ctrl', raw: 'ctrl+o' }) + ) + expect(setBell).toHaveBeenCalledWith(false) + }) + + it('reapplies the latest value on each invocation (mtime-reload semantics)', async () => { + const gw = makeFakeGw({ config: { display: {}, voice: { record_key: 'ctrl+b' } } }) + const setBell = vi.fn() + const setVoiceRecordKey = vi.fn() + + await hydrateFullConfig(gw, setBell, setVoiceRecordKey) + expect(setVoiceRecordKey).toHaveBeenLastCalledWith(expect.objectContaining({ ch: 'b' })) + + // Simulate a config edit: gw now returns a new shortcut. + gw.request = vi.fn(() => Promise.resolve({ config: { display: {}, voice: { record_key: 'alt+space' } } })) + + await hydrateFullConfig(gw, setBell, setVoiceRecordKey) + expect(setVoiceRecordKey).toHaveBeenLastCalledWith( + expect.objectContaining({ ch: 'space', mod: 'alt', named: 'space' }) + ) + }) + + it('leaves cached voiceRecordKey untouched when the RPC fails', async () => { + const gw = { request: vi.fn(() => Promise.reject(new Error('boom'))), on: vi.fn(), off: vi.fn() } as any + const setBell = vi.fn() + const setVoiceRecordKey = vi.fn() + + const result = await hydrateFullConfig(gw, setBell, setVoiceRecordKey) + + // quietRpc() swallows the error and returns null; applyDisplay + // sees cfg=null and skips the voice setter (Copilot round-8). + expect(result).toBeNull() + expect(setVoiceRecordKey).not.toHaveBeenCalled() + // bell setter still fires — applyDisplay's null-cfg path applies + // the documented bell default (false). + expect(setBell).toHaveBeenCalledWith(false) + }) + + it('threads through without a voice setter (back-compat call sites)', async () => { + const gw = makeFakeGw({ config: { display: { bell_on_complete: true } } }) + const setBell = vi.fn() + + // No third arg — applyDisplay must not throw and must still apply + // display flags (round-2 / round-8 invariant). + await expect(hydrateFullConfig(gw, setBell)).resolves.toBeTruthy() + expect(setBell).toHaveBeenCalledWith(true) + }) +}) diff --git a/ui-tui/src/app/interfaces.ts b/ui-tui/src/app/interfaces.ts index baf637aa25..dfe88fc040 100644 --- a/ui-tui/src/app/interfaces.ts +++ b/ui-tui/src/app/interfaces.ts @@ -4,6 +4,7 @@ import type { MutableRefObject, ReactNode, RefObject, SetStateAction } from 'rea import type { PasteEvent } from '../components/textInput.js' import type { GatewayClient } from '../gatewayClient.js' import type { ImageAttachResponse } from '../gatewayTypes.js' +import type { ParsedVoiceRecordKey } from '../lib/platform.js' import type { RpcResult } from '../lib/rpc.js' import type { Theme } from '../theme.js' import type { @@ -210,6 +211,7 @@ export interface InputHandlerContext { } voice: { enabled: boolean + recordKey: ParsedVoiceRecordKey recording: boolean setProcessing: StateSetter setRecording: StateSetter @@ -291,6 +293,7 @@ export interface SlashHandlerContext { } voice: { setVoiceEnabled: StateSetter + setVoiceRecordKey: (v: ParsedVoiceRecordKey) => void } } @@ -318,6 +321,7 @@ export interface AppLayoutComposerProps { queuedDisplay: string[] submit: (value: string) => void updateInput: StateSetter + voiceRecordKey: ParsedVoiceRecordKey } export interface AppLayoutProgressProps { diff --git a/ui-tui/src/app/slash/commands/session.ts b/ui-tui/src/app/slash/commands/session.ts index 0a5324ef55..a75419c3b0 100644 --- a/ui-tui/src/app/slash/commands/session.ts +++ b/ui-tui/src/app/slash/commands/session.ts @@ -10,6 +10,7 @@ import type { SessionUsageResponse, VoiceToggleResponse } from '../../../gatewayTypes.js' +import { formatVoiceRecordKey, parseVoiceRecordKey } from '../../../lib/platform.js' import { fmtK } from '../../../lib/text.js' import type { PanelSection } from '../../../types.js' import { DEFAULT_INDICATOR_STYLE, INDICATOR_STYLES, type IndicatorStyle } from '../../interfaces.js' @@ -221,6 +222,30 @@ export const sessionCommands: SlashCommand[] = [ ctx.guarded(r => { ctx.voice.setVoiceEnabled(!!r.enabled) + // Render the configured record key (config.yaml ``voice.record_key``) + // instead of hardcoded "Ctrl+B" — the gateway response carries the + // current value so /voice status and /voice on stay in sync with + // both the CLI and the TUI's actual binding (#18994). + // + // Copilot review on #19835 caught that rendering from the fresh + // backend response WITHOUT updating the frontend ``voice.recordKey`` + // state would skew display and binding between config-edit and + // the next ``mtime`` poll (~5s). Parse once, push into state so + // ``useInputHandlers()`` picks up the new binding immediately. + // + // Round-2 follow-up: only push state when the response actually + // carries ``record_key`` — otherwise an older gateway (or a future + // branch that forgets to include it) would clobber a custom user + // binding back to the default on every /voice invocation. The + // label still falls back to the documented default for display. + const parsed = r.record_key ? parseVoiceRecordKey(r.record_key) : undefined + + if (parsed) { + ctx.voice.setVoiceRecordKey(parsed) + } + + const recordKeyLabel = formatVoiceRecordKey(parsed ?? parseVoiceRecordKey('ctrl+b')) + // Match CLI's _show_voice_status / _enable_voice_mode / // _toggle_voice_tts output shape so users don't have to learn // two vocabularies. @@ -230,11 +255,11 @@ export const sessionCommands: SlashCommand[] = [ ctx.transcript.sys('Voice Mode Status') ctx.transcript.sys(` Mode: ${mode}`) ctx.transcript.sys(` TTS: ${tts}`) - ctx.transcript.sys(' Record key: Ctrl+B') + ctx.transcript.sys(` Record key: ${recordKeyLabel}`) // CLI's "Requirements:" block — surfaces STT/audio setup issues // so the user sees "STT provider: MISSING ..." instead of - // silently failing on every Ctrl+B press. + // silently failing on every record-key press. if (r.details) { ctx.transcript.sys('') ctx.transcript.sys(' Requirements:') @@ -259,7 +284,7 @@ export const sessionCommands: SlashCommand[] = [ if (r.enabled) { const tts = r.tts ? ' (TTS enabled)' : '' ctx.transcript.sys(`Voice mode enabled${tts}`) - ctx.transcript.sys(' Ctrl+B to start/stop recording') + ctx.transcript.sys(` ${recordKeyLabel} to start/stop recording`) ctx.transcript.sys(' /voice tts to toggle speech output') ctx.transcript.sys(' /voice off to disable voice mode') } else { diff --git a/ui-tui/src/app/useConfigSync.ts b/ui-tui/src/app/useConfigSync.ts index ad8f52f148..b0e590ee2c 100644 --- a/ui-tui/src/app/useConfigSync.ts +++ b/ui-tui/src/app/useConfigSync.ts @@ -7,6 +7,11 @@ import type { ConfigMtimeResponse, ReloadMcpResponse } from '../gatewayTypes.js' +import { + DEFAULT_VOICE_RECORD_KEY, + parseVoiceRecordKey, + type ParsedVoiceRecordKey +} from '../lib/platform.js' import { asRpcResult } from '../lib/rpc.js' import { @@ -89,10 +94,47 @@ const quietRpc = async = Record>( } } -export const applyDisplay = (cfg: ConfigFullResponse | null, setBell: (v: boolean) => void) => { +const _voiceRecordKeyFromConfig = (cfg: ConfigFullResponse | null): ParsedVoiceRecordKey => { + const raw = cfg?.config?.voice?.record_key + + return raw ? parseVoiceRecordKey(raw) : DEFAULT_VOICE_RECORD_KEY +} + +/** Fetch ``config.get full`` and fan the result through ``applyDisplay``. + * + * Extracted so the mtime-reload path can be exercised by the test + * suite without a React runtime (Copilot round-12 review on #19835). + * Both the initial hydration and the mtime poller use this shared + * helper, so a regression in the fetch/apply plumbing now fails the + * useConfigSync tests instead of only being visible at runtime. */ +export async function hydrateFullConfig( + gw: GatewayClient, + setBell: (v: boolean) => void, + setVoiceRecordKey?: (v: ParsedVoiceRecordKey) => void +): Promise { + const cfg = await quietRpc(gw, 'config.get', { key: 'full' }) + applyDisplay(cfg, setBell, setVoiceRecordKey) + return cfg +} + +export const applyDisplay = ( + cfg: ConfigFullResponse | null, + setBell: (v: boolean) => void, + setVoiceRecordKey?: (v: ParsedVoiceRecordKey) => void +) => { const d = cfg?.config?.display ?? {} setBell(!!d.bell_on_complete) + // Only push the voice record key when the RPC actually returned a + // config payload. ``quietRpc()`` collapses failures to ``null``; if we + // reset the cached shortcut on every null we would clobber a custom + // binding after one transient RPC error until the next config edit + // (Copilot round-8 review on #19835). The mtime-poll loop advances + // ``mtimeRef`` before this call, so staying silent on null preserves + // the last-good state and lets the next successful poll refresh it. + if (setVoiceRecordKey && cfg) { + setVoiceRecordKey(_voiceRecordKeyFromConfig(cfg)) + } patchUiState({ busyInputMode: normalizeBusyInputMode(d.busy_input_mode), compact: !!d.tui_compact, @@ -109,7 +151,13 @@ export const applyDisplay = (cfg: ConfigFullResponse | null, setBell: (v: boolea }) } -export function useConfigSync({ gw, setBellOnComplete, setVoiceEnabled, sid }: UseConfigSyncOptions) { +export function useConfigSync({ + gw, + setBellOnComplete, + setVoiceEnabled, + setVoiceRecordKey, + sid +}: UseConfigSyncOptions) { const mtimeRef = useRef(0) useEffect(() => { @@ -125,8 +173,8 @@ export function useConfigSync({ gw, setBellOnComplete, setVoiceEnabled, sid }: U quietRpc(gw, 'config.get', { key: 'mtime' }).then(r => { mtimeRef.current = Number(r?.mtime ?? 0) }) - quietRpc(gw, 'config.get', { key: 'full' }).then(r => applyDisplay(r, setBellOnComplete)) - }, [gw, setBellOnComplete, setVoiceEnabled, sid]) + void hydrateFullConfig(gw, setBellOnComplete, setVoiceRecordKey) + }, [gw, setBellOnComplete, setVoiceEnabled, setVoiceRecordKey, sid]) useEffect(() => { if (!sid) { @@ -154,17 +202,18 @@ export function useConfigSync({ gw, setBellOnComplete, setVoiceEnabled, sid }: U quietRpc(gw, 'reload.mcp', { session_id: sid, confirm: true }).then( r => r && turnController.pushActivity('MCP reloaded after config change') ) - quietRpc(gw, 'config.get', { key: 'full' }).then(r => applyDisplay(r, setBellOnComplete)) + void hydrateFullConfig(gw, setBellOnComplete, setVoiceRecordKey) }) }, MTIME_POLL_MS) return () => clearInterval(id) - }, [gw, setBellOnComplete, sid]) + }, [gw, setBellOnComplete, setVoiceRecordKey, sid]) } export interface UseConfigSyncOptions { gw: GatewayClient setBellOnComplete: (v: boolean) => void setVoiceEnabled: (v: boolean) => void + setVoiceRecordKey?: (v: ParsedVoiceRecordKey) => void sid: null | string } diff --git a/ui-tui/src/app/useInputHandlers.ts b/ui-tui/src/app/useInputHandlers.ts index a74c9e8431..20e9b087a4 100644 --- a/ui-tui/src/app/useInputHandlers.ts +++ b/ui-tui/src/app/useInputHandlers.ts @@ -348,9 +348,17 @@ export function useInputHandlers(ctx: InputHandlerContext): InputHandlerResult { return scrollTranscript(key.pageUp ? -step : step) } - // Queue-edit cancel beats selection-clear: the queue header explicitly - // promises "Esc cancel", so honoring it takes priority over the implicit - // selection-dismissal convention. Without an active edit, fall through. + // Escape-based voice bindings (ctrl/alt/super+escape) must win before the + // generic Esc handlers below; otherwise queue-edit cancel / selection-clear + // would swallow the chord and /voice would advertise a shortcut that never + // actually toggles recording in those UI states. + if (key.escape && isVoiceToggleKey(key, ch, voice.recordKey)) { + return voiceRecordToggle() + } + + // Queue-edit cancel beats selection-clear for plain Esc: the queue header + // explicitly promises "Esc cancel", so honoring it takes priority over the + // implicit selection-dismissal convention. Without an active edit, fall through. if (key.escape && cState.queueEditIdx !== null) { return cActions.clearIn() } @@ -439,7 +447,7 @@ export function useInputHandlers(ctx: InputHandlerContext): InputHandlerResult { return } - if (isVoiceToggleKey(key, ch)) { + if (isVoiceToggleKey(key, ch, voice.recordKey)) { return voiceRecordToggle() } diff --git a/ui-tui/src/app/useMainApp.ts b/ui-tui/src/app/useMainApp.ts index 218654f531..b39cc29a32 100644 --- a/ui-tui/src/app/useMainApp.ts +++ b/ui-tui/src/app/useMainApp.ts @@ -18,7 +18,7 @@ import { useGitBranch } from '../hooks/useGitBranch.js' import { useVirtualHistory } from '../hooks/useVirtualHistory.js' import { appendTranscriptMessage } from '../lib/messages.js' import { composerPromptWidth } from '../lib/inputMetrics.js' -import { isMac } from '../lib/platform.js' +import { DEFAULT_VOICE_RECORD_KEY, isMac, type ParsedVoiceRecordKey } from '../lib/platform.js' import { asRpcResult, rpcErrorMessage } from '../lib/rpc.js' import { terminalParityHints } from '../lib/terminalParity.js' import { buildToolTrailLine, sameToolTrailGroup, toolTrailLabel } from '../lib/text.js' @@ -104,6 +104,7 @@ export function useMainApp(gw: GatewayClient) { const [voiceEnabled, setVoiceEnabled] = useState(false) const [voiceRecording, setVoiceRecording] = useState(false) const [voiceProcessing, setVoiceProcessing] = useState(false) + const [voiceRecordKey, setVoiceRecordKey] = useState(DEFAULT_VOICE_RECORD_KEY) const [sessionStartedAt, setSessionStartedAt] = useState(() => Date.now()) const [turnStartedAt, setTurnStartedAt] = useState(null) const [goodVibesTick, setGoodVibesTick] = useState(0) @@ -394,7 +395,7 @@ export function useMainApp(gw: GatewayClient) { } }, [ui.busy]) - useConfigSync({ gw, setBellOnComplete, setVoiceEnabled, sid: ui.sid }) + useConfigSync({ gw, setBellOnComplete, setVoiceEnabled, setVoiceRecordKey, sid: ui.sid }) // Tab title: `⚠` waiting on approval/sudo/secret/clarify, `⏳` busy, `✓` idle. const model = ui.info?.model?.replace(/^.*\//, '') ?? '' @@ -539,6 +540,7 @@ export function useMainApp(gw: GatewayClient) { terminal: { hasSelection, scrollRef, scrollWithSelection, selection, stdout }, voice: { enabled: voiceEnabled, + recordKey: voiceRecordKey, recording: voiceRecording, setProcessing: setVoiceProcessing, setRecording: setVoiceRecording, @@ -642,7 +644,7 @@ export function useMainApp(gw: GatewayClient) { }, slashFlightRef, transcript: { page, panel, send, setHistoryItems, sys, trimLastExchange: session.trimLastExchange }, - voice: { setVoiceEnabled } + voice: { setVoiceEnabled, setVoiceRecordKey } }), [ catalog, @@ -782,9 +784,10 @@ export function useMainApp(gw: GatewayClient) { queueEditIdx: composerState.queueEditIdx, queuedDisplay: composerState.queuedDisplay, submit, - updateInput: composerActions.setInput + updateInput: composerActions.setInput, + voiceRecordKey }), - [cols, composerActions, composerState, empty, pagerPageSize, submit] + [cols, composerActions, composerState, empty, pagerPageSize, submit, voiceRecordKey] ) // Pass current progress through unfrozen — streaming update throttling diff --git a/ui-tui/src/components/appLayout.tsx b/ui-tui/src/components/appLayout.tsx index 8c2d210ca1..ec60726ed3 100644 --- a/ui-tui/src/components/appLayout.tsx +++ b/ui-tui/src/components/appLayout.tsx @@ -288,6 +288,7 @@ const ComposerPane = memo(function ComposerPane({ onSubmit={composer.submit} placeholder={composer.empty ? PLACEHOLDER : ui.busy ? 'Ctrl+C to interrupt…' : ''} value={composer.input} + voiceRecordKey={composer.voiceRecordKey} /> diff --git a/ui-tui/src/components/textInput.tsx b/ui-tui/src/components/textInput.tsx index 3008f0baf4..d8151e72b7 100644 --- a/ui-tui/src/components/textInput.tsx +++ b/ui-tui/src/components/textInput.tsx @@ -5,7 +5,14 @@ import { type MutableRefObject, useEffect, useMemo, useRef, useState } from 'rea import { setInputSelection } from '../app/inputSelectionStore.js' import { readClipboardText, writeClipboardText } from '../lib/clipboard.js' import { cursorLayout, offsetFromPosition } from '../lib/inputMetrics.js' -import { isActionMod, isMac, isMacActionFallback } from '../lib/platform.js' +import { + DEFAULT_VOICE_RECORD_KEY, + isActionMod, + isMac, + isMacActionFallback, + isVoiceToggleKey, + type ParsedVoiceRecordKey +} from '../lib/platform.js' type InkExt = typeof Ink & { stringWidth: (s: string) => number @@ -239,6 +246,7 @@ export function TextInput({ onSubmit, mask, mouseApiRef, + voiceRecordKey = DEFAULT_VOICE_RECORD_KEY, placeholder = '', focus = true }: TextInputProps) { @@ -699,6 +707,15 @@ export function TextInput({ (inp: string, k: Key, event: InputEvent) => { const eventRaw = event.keypress.raw + // Configured voice shortcut wins over composer-level defaults like + // paste/copy so users who bind voice to ctrl+v / alt+v / cmd+v + // actually get voice toggled instead of a paste (Copilot round-7 + // follow-up on #19835). The pass-through predicate is a no-op for + // ordinary typing and plain paste when voice is unbound to 'v'. + if (shouldPassThroughToGlobalHandler(inp, k, voiceRecordKey)) { + return + } + if ( eventRaw === '\x1bv' || eventRaw === '\x1bV' || @@ -744,22 +761,6 @@ export function TextInput({ return } - // Ctrl chords claimed by useInputHandlers — pass through instead of - // letting them fall into readline-style nav or a literal char insert. - // Ctrl+B = voice toggle, Ctrl+X = delete queued message while editing. - if ( - (k.ctrl && inp === 'c') || - (k.ctrl && inp === 'b') || - (k.ctrl && inp === 'x') || - k.tab || - (k.shift && k.tab) || - k.pageUp || - k.pageDown || - k.escape - ) { - return - } - if (k.return) { if (k.shift || k.ctrl || (isMac ? isActionMod(k) : k.meta)) { flushParentChange() @@ -1041,8 +1042,23 @@ interface TextInputProps { onSubmit?: (v: string) => void placeholder?: string value: string + voiceRecordKey?: ParsedVoiceRecordKey } +export const shouldPassThroughToGlobalHandler = ( + input: string, + key: Key, + voiceRecordKey: ParsedVoiceRecordKey = DEFAULT_VOICE_RECORD_KEY +): boolean => + (key.ctrl && input === 'c') || + (key.ctrl && input === 'x') || + key.tab || + (key.shift && key.tab) || + key.pageUp || + key.pageDown || + key.escape || + isVoiceToggleKey(key, input, voiceRecordKey) + export interface TextInputMouseApi { dragAt: (row: number, col: number) => void end: () => void diff --git a/ui-tui/src/gatewayTypes.ts b/ui-tui/src/gatewayTypes.ts index a1513d2a6e..7fca2837fa 100644 --- a/ui-tui/src/gatewayTypes.ts +++ b/ui-tui/src/gatewayTypes.ts @@ -75,8 +75,14 @@ export interface ConfigDisplayConfig { tui_statusbar?: 'bottom' | 'off' | 'on' | 'top' | boolean } +export interface ConfigVoiceConfig { + // Raw `yaml.safe_load()` value from config; may be non-string if hand-edited. + // Callers must normalize/validate at runtime (parseVoiceRecordKey()). + record_key?: unknown +} + export interface ConfigFullResponse { - config?: { display?: ConfigDisplayConfig } + config?: { display?: ConfigDisplayConfig; voice?: ConfigVoiceConfig } } export interface ConfigMtimeResponse { @@ -279,6 +285,7 @@ export interface VoiceToggleResponse { available?: boolean details?: string enabled?: boolean + record_key?: string stt_available?: boolean tts?: boolean } diff --git a/ui-tui/src/lib/platform.ts b/ui-tui/src/lib/platform.ts index 343d8f8683..d7d2cc1ff0 100644 --- a/ui-tui/src/lib/platform.ts +++ b/ui-tui/src/lib/platform.ts @@ -51,13 +51,359 @@ export const isCopyShortcut = ( (isMac && key.ctrl && (key.meta || key.super === true))) /** - * Voice recording toggle key (Ctrl+B). + * Voice recording toggle key — configurable via ``voice.record_key`` in + * ``config.yaml`` (default ``ctrl+b``). * - * Documented as "Ctrl+B" everywhere: tips.py, config.yaml's voice.record_key - * default, and the Python CLI prompt_toolkit handler. We accept raw Ctrl+B on - * every platform so the TUI matches those docs. On macOS we additionally - * accept Cmd+B (the platform action modifier) so existing macOS muscle memory - * keeps working. + * Documented in tips.py, the Python CLI prompt_toolkit handler, and the + * config.yaml default. The TUI honours the same config knob (#18994); + * when ``voice.record_key`` is e.g. ``ctrl+o`` the TUI binds Ctrl+O. + * + * Only the documented default (``ctrl+b``) additionally accepts the + * macOS action modifier (Cmd+B) — custom bindings like ``ctrl+o`` + * require the literal Ctrl bit so Cmd+O can't steal the shortcut. */ -export const isVoiceToggleKey = (key: { ctrl: boolean; meta: boolean; super?: boolean }, ch: string): boolean => - (key.ctrl || isActionMod(key)) && ch.toLowerCase() === 'b' +export type VoiceRecordKeyMod = 'alt' | 'ctrl' | 'super' + +/** Named (multi-character) keys we support, matching the CLI's + * prompt_toolkit binding shape (``c-space``, ``c-enter``, etc.) so a + * config value like ``ctrl+space`` binds in both runtimes. */ +export type VoiceRecordKeyNamed = 'backspace' | 'delete' | 'enter' | 'escape' | 'space' | 'tab' + +export interface ParsedVoiceRecordKey { + /** Single character (``'b'``, ``'o'``) when ``named`` is undefined, + * otherwise the named-key token (``'space'``, ``'enter'``…). Kept as + * one field for back-compat with the v1 ``{ ch, mod, raw }`` shape. */ + ch: string + mod: VoiceRecordKeyMod + named?: VoiceRecordKeyNamed + raw: string +} + +export const DEFAULT_VOICE_RECORD_KEY: ParsedVoiceRecordKey = { + ch: 'b', + mod: 'ctrl', + raw: 'ctrl+b' +} + +/** Modifier aliases. + * + * ``meta`` / ``cmd`` / ``command`` are intentionally absent. + * hermes-ink sets ``key.meta`` for plain Alt/Option on every platform + * AND for Cmd on some legacy macOS terminals (Terminal.app without + * kitty-protocol passthrough). Accepting any of those as a literal + * modifier would produce a display/binding mismatch — a config like + * ``cmd+b`` would render as ``Cmd+B`` but silently fire on Alt+B, or + * never fire at all on legacy terminals even though the UI advertises + * it (Copilot round-6 review on #19835). Users on modern kitty-style + * terminals (iTerm2 CSI-u, Ghostty, Kitty, WezTerm, Alacritty) spell + * the platform action modifier ``super`` / ``win``, which match the + * unambiguous ``key.super`` bit. macOS users on Terminal.app stick + * with the documented ``ctrl+b``. + * + * Cross-runtime parity: the ``ctrl`` / ``control`` / ``alt`` / ``option`` / + * ``opt`` spellings are normalized identically in the classic CLI + * (``hermes_cli/voice.py::normalize_voice_record_key_for_prompt_toolkit``) + * so one ``voice.record_key`` value binds the same shortcut in both + * runtimes (Copilot round-9 review on #19835). The ``super`` / + * ``win`` / ``windows`` spellings are TUI-only — prompt_toolkit has no + * super modifier, so the CLI falls back to the documented default and + * logs a warning at startup (Copilot round-11 review on #19835). */ +const _MOD_ALIASES: Record = { + alt: 'alt', + control: 'ctrl', + ctrl: 'ctrl', + option: 'alt', + opt: 'alt', + super: 'super', + win: 'super', + windows: 'super' +} + +/** Map config-string named tokens to the canonical name used at match time. + * + * Aliases mirror what prompt_toolkit accepts (``return`` ↔ ``enter``, + * ``esc`` ↔ ``escape``) so a config that round-trips through the CLI also + * binds in the TUI. */ +const _NAMED_KEY_ALIASES: Record = { + backspace: 'backspace', + bs: 'backspace', + del: 'delete', + delete: 'delete', + enter: 'enter', + esc: 'escape', + escape: 'escape', + ret: 'enter', + return: 'enter', + space: 'space', + spc: 'space', + tab: 'tab' +} + +/** ``useInputHandlers()`` intercepts these unconditionally before the + * voice check runs, so a binding like ``ctrl+c`` (interrupt), + * ``ctrl+d`` (quit), or ``ctrl+l`` (clear screen) would be advertised + * in /voice status but never fire push-to-talk. Reject at parse time + * so the user gets the documented Ctrl+B instead of a dead shortcut + * (Copilot round-4 review on #19835). + * + * ``ctrl+x`` is intentionally NOT here — it's only claimed during + * queue-edit (``queueEditIdx !== null``), so the voice binding works + * for most of the session and matches CLI parity for ``ctrl+`` + * bindings (Copilot round-8 review on #19835). */ +const _RESERVED_CTRL_CHARS = new Set(['c', 'd', 'l']) + +/** On macOS the action-modifier intercepts these editor chords via + * ``isCopyShortcut`` / ``isAction`` in ``useInputHandlers()``: + * - super+c → copy + * - super+d → exit + * - super+l → clear screen + * - super+v → paste (also claimed at the TextInput layer) + * On Linux/Windows those globals key off Ctrl instead of Super, so + * super+ bindings don't collide. Gate the rejection to darwin + * at parse time so kitty/CSI-u ``super+`` configs still work for + * non-mac users (Copilot round-8 review on #19835). */ +const _RESERVED_SUPER_CHARS = new Set(['c', 'd', 'l', 'v']) + +/** On macOS ``isActionMod`` accepts ``key.meta`` as the action + * modifier — but hermes-ink reports Alt as ``key.meta`` on many + * terminals. So on darwin a configured ``alt+c`` / ``alt+d`` / ``alt+l`` + * gets swallowed by ``isCopyShortcut`` / ``isAction`` before the voice + * check runs. Block at parse time so /voice status doesn't advertise + * a shortcut that actually copies / quits / clears (Copilot round-12 + * review on #19835). */ +const _RESERVED_ALT_CHARS_MAC = new Set(['c', 'd', 'l']) + +interface RuntimeKeyEvent { + alt?: boolean + backspace?: boolean + ctrl: boolean + delete?: boolean + escape?: boolean + meta: boolean + return?: boolean + shift?: boolean + super?: boolean + tab?: boolean +} + +/** Match an ink ``key`` event against a parsed named key. The ink runtime + * sets one boolean per named key; ``space`` is a printable char so it + * arrives as ``ch === ' '`` rather than a dedicated ``key.space`` flag. */ +const _matchesNamedKey = ( + named: VoiceRecordKeyNamed, + key: RuntimeKeyEvent, + ch: string +): boolean => { + switch (named) { + case 'backspace': + return key.backspace === true + case 'delete': + return key.delete === true + case 'enter': + return key.return === true + case 'escape': + return key.escape === true + case 'space': + return ch === ' ' + case 'tab': + return key.tab === true + } +} + +/** + * Parse a config-string voice record key like ``ctrl+b`` / ``alt+r`` / + * ``ctrl+space`` into ``{mod, ch, named?}``. Accepts single characters + * AND the named tokens declared in ``_NAMED_KEY_ALIASES`` (``space``, + * ``enter``/``return``, ``tab``, ``escape``/``esc``, ``backspace``, + * ``delete``) — matching the keys prompt_toolkit accepts on the CLI + * side via the ``c-`` rewrite in ``cli.py``. + * + * Accepts ``unknown`` because the source is raw YAML via + * ``config.get full`` — a hand-edited ``voice.record_key: 1`` or + * ``voice.record_key: true`` would otherwise crash ``.trim()`` on a + * non-string scalar (Copilot round-3 review on #19835). Non-string / + * empty / unrecognised values fall back to the documented Ctrl+B + * default so a typo never silently disables the shortcut. + */ +export const parseVoiceRecordKey = (raw: unknown): ParsedVoiceRecordKey => { + if (typeof raw !== 'string') { + return DEFAULT_VOICE_RECORD_KEY + } + + const lower = raw.trim().toLowerCase() + + if (!lower) { + return DEFAULT_VOICE_RECORD_KEY + } + + const parts = lower.split('+').map(p => p.trim()).filter(Boolean) + + if (!parts.length) { + return DEFAULT_VOICE_RECORD_KEY + } + + const last = parts[parts.length - 1] + const modCandidates = parts.slice(0, -1) + + // Reject multi-modifier chords (``ctrl+alt+r``, ``cmd+ctrl+b``) rather + // than silently dropping the extra modifier — the previous + // single-token validator made a typo bind a different shortcut than + // the user configured (Copilot round-3 review on #19835). The classic + // CLI only supports single-modifier bindings via prompt_toolkit's + // ``c-x`` / ``a-x`` rewrite in ``cli.py``, so this matches CLI parity. + if (modCandidates.length > 1) { + return DEFAULT_VOICE_RECORD_KEY + } + + // Require an explicit modifier. A bare ``o`` / ``space`` / ``escape`` + // has no sensible mapping: the CLI's prompt_toolkit binds the raw + // key (no rewrite) so bare-char configs would silently diverge + // between the two runtimes (Copilot round-4 review on #19835). + // Fall back to the documented default. + if (modCandidates.length === 0) { + return DEFAULT_VOICE_RECORD_KEY + } + + const norm = _MOD_ALIASES[modCandidates[0]] + + // Unknown modifier token (e.g. bare ``meta+b`` which is ambiguous on + // the wire) falls back to the documented default rather than + // silently coercing to Ctrl and producing a misleading bind. + if (!norm) { + return DEFAULT_VOICE_RECORD_KEY + } + + const mod = norm + + // Block bindings the TUI input handler intercepts before the voice + // check — ``ctrl+c`` / ``ctrl+d`` / ``ctrl+l`` would never actually + // fire push-to-talk, so advertising them in /voice status is a lie. + if (mod === 'ctrl' && last.length === 1 && _RESERVED_CTRL_CHARS.has(last)) { + return DEFAULT_VOICE_RECORD_KEY + } + + // Same for ``super+c`` / ``super+d`` / ``super+l`` / ``super+v`` on + // macOS only — those are copy / exit / clear / paste and get claimed + // by ``isCopyShortcut`` / ``isAction`` / the TextInput paste layer + // before voice has a chance to toggle. On Linux/Windows the TUI + // globals key off Ctrl (not Super), so kitty/CSI-u ``super+`` + // bindings stay usable for non-mac users. + if (isMac && mod === 'super' && last.length === 1 && _RESERVED_SUPER_CHARS.has(last)) { + return DEFAULT_VOICE_RECORD_KEY + } + + // On macOS hermes-ink reports Alt as ``key.meta``, which ``isActionMod`` + // accepts as the mac action modifier. So ``alt+c`` / ``alt+d`` / ``alt+l`` + // collide with copy / exit / clear in ``useInputHandlers()`` before the + // voice check. Reject at parse time on darwin only — non-mac ``alt+`` + // bindings are still usable (Copilot round-12 review on #19835). + if (isMac && mod === 'alt' && last.length === 1 && _RESERVED_ALT_CHARS_MAC.has(last)) { + return DEFAULT_VOICE_RECORD_KEY + } + + if (last.length === 1) { + return { ch: last, mod, raw: lower } + } + + const named = _NAMED_KEY_ALIASES[last] + + if (named) { + return { ch: named, mod, named, raw: lower } + } + + // Unknown multi-character token (e.g. typo'd ``ctrl+spcae``) — fall back + // to the doc default rather than silently disabling the binding. + return DEFAULT_VOICE_RECORD_KEY +} + +/** Render a parsed key back as ``Ctrl+B`` / ``Ctrl+Space`` for status text. + * + * Platform-aware for the ``super`` modifier: renders ``Cmd`` on macOS and + * ``Super`` elsewhere. Previously rendered ``Cmd`` universally, which told + * Linux/Windows users the wrong modifier to press (Copilot review, round + * 2 on #19835). */ +export const formatVoiceRecordKey = (parsed: ParsedVoiceRecordKey): string => { + const modLabel = + parsed.mod === 'super' ? (isMac ? 'Cmd' : 'Super') : parsed.mod[0].toUpperCase() + parsed.mod.slice(1) + // Named tokens render in title case (Ctrl+Space, Ctrl+Enter); single + // chars render upper-case to match the existing Ctrl+B convention. + const keyLabel = parsed.named + ? parsed.named[0].toUpperCase() + parsed.named.slice(1) + : parsed.ch.toUpperCase() + + return `${modLabel}+${keyLabel}` +} + +/** Whether the parsed binding is the documented default (ctrl+b). + * + * Compare on the parsed spec rather than ``raw`` so semantically-equal + * aliases (``control+b``, ``ctrl + b``) still get the macOS Cmd+B + * muscle-memory fallback (Copilot review, round 2 on #19835). */ +const _isDefaultVoiceKey = (parsed: ParsedVoiceRecordKey): boolean => + parsed.mod === DEFAULT_VOICE_RECORD_KEY.mod && + parsed.ch === DEFAULT_VOICE_RECORD_KEY.ch && + parsed.named === DEFAULT_VOICE_RECORD_KEY.named + +export const isVoiceToggleKey = ( + key: RuntimeKeyEvent, + ch: string, + configured: ParsedVoiceRecordKey = DEFAULT_VOICE_RECORD_KEY +): boolean => { + // Match the configured key first (single-char compare or named-key + // event-property check). Bail out before evaluating modifier shape + // so the wrong key never reaches the modifier guard. + if (configured.named) { + if (!_matchesNamedKey(configured.named, key, ch)) { + return false + } + } else if (ch.toLowerCase() !== configured.ch) { + return false + } + + // The parser rejects multi-modifier configs (``ctrl+shift+b`` etc.), + // so at match time Shift must always be clear — otherwise + // ``ctrl+tab`` would also fire on Ctrl+Shift+Tab and ``alt+enter`` + // on Alt+Shift+Enter, triggering a different chord than configured + // (Copilot round-5 review on #19835). + if (key.shift === true) { + return false + } + + switch (configured.mod) { + case 'alt': + // Most terminals surface Alt as either ``alt`` or ``meta``; accept + // both so the binding works across xterm-style and kitty-style + // protocols. Guard against ctrl/super bits so a chord like + // Ctrl+Alt+ or Cmd+Alt+ doesn't spuriously fire the + // alt binding. + // + // Bare Escape on hermes-ink can arrive as ``key.meta=true`` on some + // terminals, so a configured ``alt+escape`` must not match that shape; + // require an explicit alt bit for escape chords (Copilot round-7 + // follow-up on #19835). + return (key.alt === true || (key.meta && key.escape !== true)) && !key.ctrl && key.super !== true + case 'ctrl': + // Require the Ctrl bit AND a clear Alt/Super so a chord like + // Ctrl+Alt+ / Ctrl+Cmd+ doesn't spuriously match + // ``ctrl+`` (Copilot round-6 review on #19835). + // + // The documented default (``ctrl+b``) additionally accepts the + // explicit ``key.super`` bit on macOS for Cmd+B muscle memory — + // but ONLY ``key.super`` (kitty-style), never ``key.meta``, since + // ``key.meta`` is hermes-ink's Alt signal and accepting it would + // fire the binding on Alt+B. + if (key.ctrl) { + return !key.alt && !key.meta && key.super !== true + } + + return _isDefaultVoiceKey(configured) && isMac && key.super === true && !key.alt && !key.meta + case 'super': + // Require the explicit ``key.super`` bit (kitty-style protocol) + // AND clear Ctrl/Alt/Meta so Ctrl+Cmd+X or Alt+Cmd+X don't + // spuriously fire the super binding (Copilot round-6 review on + // #19835). Legacy-terminal users whose Cmd arrives as + // ``key.meta`` need a kitty-protocol terminal — see the + // _MOD_ALIASES doc-comment for the rationale. + return key.super === true && !key.ctrl && !key.alt && !key.meta + } +} From b6322901664c0af138035d6f753feedeb1d2e8b7 Mon Sep 17 00:00:00 2001 From: helix4u <4317663+helix4u@users.noreply.github.com> Date: Mon, 4 May 2026 13:35:23 -0600 Subject: [PATCH 09/21] fix(gateway): handle planned service stops --- gateway/run.py | 41 ++++-- gateway/status.py | 158 +++++++++++++++-------- hermes_cli/gateway.py | 20 +++ tests/gateway/test_status.py | 85 ++++++++++++ tests/hermes_cli/test_gateway_service.py | 54 ++++++-- 5 files changed, 286 insertions(+), 72 deletions(-) diff --git a/gateway/run.py b/gateway/run.py index 2b085d9915..ebfd2731fe 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -15003,15 +15003,14 @@ async def start_gateway(config: Optional[GatewayConfig] = None, replace: bool = runner = GatewayRunner(config) - # Track whether a signal initiated the shutdown (vs. internal request). - # When an unexpected SIGTERM kills the gateway, we exit non-zero so - # systemd's Restart=on-failure revives the process. systemctl stop - # is safe: systemd tracks stop-requested state independently of exit - # code, so Restart= never fires for a deliberate stop. + # Track whether an unexpected signal initiated the shutdown. When an + # unexpected SIGTERM kills the gateway, we exit non-zero so service + # managers can revive the process. Planned stop paths write a marker + # before signalling us so they can exit cleanly instead. _signal_initiated_shutdown = False # Set up signal handlers - def shutdown_signal_handler(): + def shutdown_signal_handler(received_signal=None): nonlocal _signal_initiated_shutdown # Planned --replace takeover check: when a sibling gateway is # taking over via --replace, it wrote a marker naming this PID @@ -15027,10 +15026,28 @@ async def start_gateway(config: Optional[GatewayConfig] = None, replace: bool = except Exception as e: logger.debug("Takeover marker check failed: %s", e) + # Planned stop check: service managers and `hermes gateway stop` + # also send SIGTERM, which is indistinguishable from an unexpected + # external kill unless the CLI marks it first. SIGINT comes from an + # interactive Ctrl+C and is likewise an intentional foreground stop. + planned_stop = False + if received_signal == signal.SIGINT: + planned_stop = True + elif not planned_takeover: + try: + from gateway.status import consume_planned_stop_marker_for_self + planned_stop = consume_planned_stop_marker_for_self() + except Exception as e: + logger.debug("Planned stop marker check failed: %s", e) + if planned_takeover: logger.info( "Received SIGTERM as a planned --replace takeover — exiting cleanly" ) + elif planned_stop: + logger.info( + "Received SIGTERM/SIGINT as a planned gateway stop — exiting cleanly" + ) else: _signal_initiated_shutdown = True logger.info("Received SIGTERM/SIGINT — initiating shutdown") @@ -15066,7 +15083,7 @@ async def start_gateway(config: Optional[GatewayConfig] = None, replace: bool = if threading.current_thread() is threading.main_thread(): for sig in (signal.SIGINT, signal.SIGTERM): try: - loop.add_signal_handler(sig, shutdown_signal_handler) + loop.add_signal_handler(sig, shutdown_signal_handler, sig) except NotImplementedError: pass if hasattr(signal, "SIGUSR1"): @@ -15164,14 +15181,14 @@ async def start_gateway(config: Optional[GatewayConfig] = None, replace: bool = if runner.exit_code is not None: raise SystemExit(runner.exit_code) - # When a signal (SIGTERM/SIGINT) caused the shutdown and it wasn't a - # planned restart (/restart, /update, SIGUSR1), exit non-zero so - # systemd's Restart=on-failure revives the process. This covers: + # When an unexpected SIGTERM caused the shutdown and it wasn't a planned + # restart (/restart, /update, SIGUSR1), exit non-zero so systemd's + # Restart=on-failure revives the process. This covers: # - hermes update killing the gateway mid-work # - External kill commands # - WSL2/container runtime sending unexpected signals - # systemctl stop is safe: systemd tracks "stop requested" state - # independently of exit code, so Restart= never fires for it. + # `hermes gateway stop` and interactive Ctrl+C are handled above as + # planned stops and should not trigger service-manager revival. if _signal_initiated_shutdown and not runner._restart_requested: logger.info( "Exiting with code 1 (signal-initiated shutdown without restart " diff --git a/gateway/status.py b/gateway/status.py index f329b25f08..bdff9aa988 100644 --- a/gateway/status.py +++ b/gateway/status.py @@ -637,6 +637,8 @@ def release_all_scoped_locks( _TAKEOVER_MARKER_FILENAME = ".gateway-takeover.json" _TAKEOVER_MARKER_TTL_S = 60 # Marker older than this is treated as stale +_PLANNED_STOP_MARKER_FILENAME = ".gateway-planned-stop.json" +_PLANNED_STOP_MARKER_TTL_S = 60 def _get_takeover_marker_path() -> Path: @@ -645,6 +647,67 @@ def _get_takeover_marker_path() -> Path: return home / _TAKEOVER_MARKER_FILENAME +def _get_planned_stop_marker_path() -> Path: + """Return the path to the intentional gateway stop marker file.""" + home = get_hermes_home() + return home / _PLANNED_STOP_MARKER_FILENAME + + +def _marker_is_stale(written_at: str, ttl_s: int) -> bool: + try: + written_dt = datetime.fromisoformat(written_at) + age = (datetime.now(timezone.utc) - written_dt).total_seconds() + return age > ttl_s + except (TypeError, ValueError): + return True + + +def _consume_pid_marker_for_self( + path: Path, + *, + pid_field: str, + start_time_field: str, + ttl_s: int, +) -> bool: + record = _read_json_file(path) + if not record: + return False + + try: + target_pid = int(record[pid_field]) + target_start_time = record.get(start_time_field) + written_at = record.get("written_at") or "" + except (KeyError, TypeError, ValueError): + try: + path.unlink(missing_ok=True) + except OSError: + pass + return False + + if _marker_is_stale(written_at, ttl_s): + try: + path.unlink(missing_ok=True) + except OSError: + pass + return False + + our_pid = os.getpid() + our_start_time = _get_process_start_time(our_pid) + matches = ( + target_pid == our_pid + and target_start_time is not None + and our_start_time is not None + and target_start_time == our_start_time + ) + + try: + path.unlink(missing_ok=True) + except OSError: + pass + + return matches + + def write_takeover_marker(target_pid: int) -> bool: """Record that ``target_pid`` is being replaced by the current process. @@ -681,59 +744,13 @@ def consume_takeover_marker_for_self() -> bool: Always unlinks the marker on match (and on detected staleness) so subsequent unrelated signals don't re-trigger. """ - path = _get_takeover_marker_path() - record = _read_json_file(path) - if not record: - return False - - # Any malformed or stale marker → drop it and return False - try: - target_pid = int(record["target_pid"]) - target_start_time = record.get("target_start_time") - written_at = record.get("written_at") or "" - except (KeyError, TypeError, ValueError): - try: - path.unlink(missing_ok=True) - except OSError: - pass - return False - - # TTL guard: a stale marker older than _TAKEOVER_MARKER_TTL_S is ignored. - stale = False - try: - written_dt = datetime.fromisoformat(written_at) - age = (datetime.now(timezone.utc) - written_dt).total_seconds() - if age > _TAKEOVER_MARKER_TTL_S: - stale = True - except (TypeError, ValueError): - stale = True # Unparseable timestamp — treat as stale - - if stale: - try: - path.unlink(missing_ok=True) - except OSError: - pass - return False - - # Does the marker name THIS process? - our_pid = os.getpid() - our_start_time = _get_process_start_time(our_pid) - matches = ( - target_pid == our_pid - and target_start_time is not None - and our_start_time is not None - and target_start_time == our_start_time + return _consume_pid_marker_for_self( + _get_takeover_marker_path(), + pid_field="target_pid", + start_time_field="target_start_time", + ttl_s=_TAKEOVER_MARKER_TTL_S, ) - # Consume the marker whether it matched or not — a marker that doesn't - # match our identity is stale-for-us anyway. - try: - path.unlink(missing_ok=True) - except OSError: - pass - - return matches - def clear_takeover_marker() -> None: """Remove the takeover marker unconditionally. Safe to call repeatedly.""" @@ -743,6 +760,45 @@ def clear_takeover_marker() -> None: pass +def write_planned_stop_marker(target_pid: int) -> bool: + """Record that ``target_pid`` is being stopped intentionally. + + The gateway exits non-zero for unexpected SIGTERM so service managers can + revive it. Service stop commands send the same SIGTERM, so the CLI writes + this short-lived marker first to let the target process exit cleanly. + """ + try: + target_start_time = _get_process_start_time(target_pid) + record = { + "target_pid": target_pid, + "target_start_time": target_start_time, + "stopper_pid": os.getpid(), + "written_at": _utc_now_iso(), + } + _write_json_file(_get_planned_stop_marker_path(), record) + return True + except (OSError, PermissionError): + return False + + +def consume_planned_stop_marker_for_self() -> bool: + """Return True when the current process is being intentionally stopped.""" + return _consume_pid_marker_for_self( + _get_planned_stop_marker_path(), + pid_field="target_pid", + start_time_field="target_start_time", + ttl_s=_PLANNED_STOP_MARKER_TTL_S, + ) + + +def clear_planned_stop_marker() -> None: + """Remove the planned-stop marker unconditionally.""" + try: + _get_planned_stop_marker_path().unlink(missing_ok=True) + except OSError: + pass + + def get_running_pid( pid_path: Optional[Path] = None, *, diff --git a/hermes_cli/gateway.py b/hermes_cli/gateway.py index c1804f9c7f..846736a2cc 100644 --- a/hermes_cli/gateway.py +++ b/hermes_cli/gateway.py @@ -785,6 +785,12 @@ def stop_profile_gateway() -> bool: if pid is None: return False + try: + from gateway.status import write_planned_stop_marker + write_planned_stop_marker(pid) + except Exception: + pass + try: os.kill(pid, signal.SIGTERM) except ProcessLookupError: @@ -2043,6 +2049,13 @@ def systemd_stop(system: bool = False): if system: _require_root_for_system_service("stop") _require_service_installed("stop", system=system) + try: + from gateway.status import get_running_pid, write_planned_stop_marker + pid = get_running_pid(cleanup_stale=False) + if pid is not None: + write_planned_stop_marker(pid) + except Exception: + pass _run_systemctl(["stop", get_service_name()], system=system, check=True, timeout=90) print(f"✓ {_service_scope_label(system).capitalize()} service stopped") @@ -2404,6 +2417,13 @@ def launchd_start(): def launchd_stop(): label = get_launchd_label() target = f"{_launchd_domain()}/{label}" + try: + from gateway.status import get_running_pid, write_planned_stop_marker + pid = get_running_pid(cleanup_stale=False) + if pid is not None: + write_planned_stop_marker(pid) + except Exception: + pass # bootout unloads the service definition so KeepAlive doesn't respawn # the process. A plain `kill SIGTERM` only signals the process — launchd # immediately restarts it because KeepAlive.SuccessfulExit = false. diff --git a/tests/gateway/test_status.py b/tests/gateway/test_status.py index 7138b6514e..e7cd0dc060 100644 --- a/tests/gateway/test_status.py +++ b/tests/gateway/test_status.py @@ -702,3 +702,88 @@ class TestTakeoverMarker: # We are not the target — must NOT consume as planned assert result is False + + +class TestPlannedStopMarker: + """Tests for intentional service/manual gateway stop markers.""" + + def test_write_marker_records_target_identity(self, tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.setattr(status, "_get_process_start_time", lambda pid: 42) + + ok = status.write_planned_stop_marker(target_pid=12345) + + assert ok is True + marker = tmp_path / ".gateway-planned-stop.json" + assert marker.exists() + payload = json.loads(marker.read_text()) + assert payload["target_pid"] == 12345 + assert payload["target_start_time"] == 42 + assert payload["stopper_pid"] == os.getpid() + assert "written_at" in payload + + def test_consume_returns_true_when_marker_names_self(self, tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.setattr(status, "_get_process_start_time", lambda pid: 100) + ok = status.write_planned_stop_marker(target_pid=os.getpid()) + assert ok is True + + result = status.consume_planned_stop_marker_for_self() + + assert result is True + assert not (tmp_path / ".gateway-planned-stop.json").exists() + + def test_consume_returns_false_for_different_pid(self, tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.setattr(status, "_get_process_start_time", lambda pid: 100) + ok = status.write_planned_stop_marker(target_pid=os.getpid() + 9999) + assert ok is True + + result = status.consume_planned_stop_marker_for_self() + + assert result is False + assert not (tmp_path / ".gateway-planned-stop.json").exists() + + def test_consume_returns_false_for_stale_marker(self, tmp_path, monkeypatch): + from datetime import datetime, timezone, timedelta + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + marker_path = tmp_path / ".gateway-planned-stop.json" + stale_time = (datetime.now(timezone.utc) - timedelta(minutes=2)).isoformat() + marker_path.write_text(json.dumps({ + "target_pid": os.getpid(), + "target_start_time": 123, + "stopper_pid": 99999, + "written_at": stale_time, + })) + monkeypatch.setattr(status, "_get_process_start_time", lambda pid: 123) + + result = status.consume_planned_stop_marker_for_self() + + assert result is False + assert not marker_path.exists() + + def test_clear_planned_stop_marker_is_idempotent(self, tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + monkeypatch.setattr(status, "_get_process_start_time", lambda pid: 100) + + status.clear_planned_stop_marker() + status.write_planned_stop_marker(target_pid=12345) + assert (tmp_path / ".gateway-planned-stop.json").exists() + + status.clear_planned_stop_marker() + + assert not (tmp_path / ".gateway-planned-stop.json").exists() + status.clear_planned_stop_marker() + + def test_write_marker_returns_false_on_write_failure(self, tmp_path, monkeypatch): + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + + def raise_oserror(*args, **kwargs): + raise OSError("simulated write failure") + + monkeypatch.setattr(status, "_write_json_file", raise_oserror) + + ok = status.write_planned_stop_marker(target_pid=12345) + + assert ok is False diff --git a/tests/hermes_cli/test_gateway_service.py b/tests/hermes_cli/test_gateway_service.py index 3e9a4d3720..994e8d0284 100644 --- a/tests/hermes_cli/test_gateway_service.py +++ b/tests/hermes_cli/test_gateway_service.py @@ -8,6 +8,7 @@ from types import SimpleNamespace import pytest import hermes_cli.gateway as gateway_cli +from gateway import status from gateway.restart import ( DEFAULT_GATEWAY_RESTART_DRAIN_TIMEOUT, GATEWAY_SERVICE_RESTART_EXIT_CODE, @@ -106,6 +107,30 @@ class TestSystemdServiceRefresh: ["systemctl", "--user", "reload-or-restart", gateway_cli.get_service_name()], ] + def test_systemd_stop_marks_running_gateway_as_planned_stop(self, monkeypatch): + calls = [] + markers = [] + + monkeypatch.setattr(gateway_cli, "_select_systemd_scope", lambda system=False: False) + monkeypatch.setattr(gateway_cli, "_require_service_installed", lambda action, system=False: None) + monkeypatch.setattr(status, "get_running_pid", lambda cleanup_stale=True: 321) + monkeypatch.setattr( + status, + "write_planned_stop_marker", + lambda pid: markers.append(pid) or True, + ) + + def fake_run_systemctl(args, **kwargs): + calls.append(args) + return SimpleNamespace(returncode=0, stdout="", stderr="") + + monkeypatch.setattr(gateway_cli, "_run_systemctl", fake_run_systemctl) + + gateway_cli.systemd_stop() + + assert markers == [321] + assert calls == [["stop", gateway_cli.get_service_name()]] + def test_run_gateway_refreshes_outdated_unit_on_boot(self, tmp_path, monkeypatch): """run_gateway() should refresh the systemd unit on boot so that @@ -127,11 +152,8 @@ class TestSystemdServiceRefresh: monkeypatch.setattr(gateway_cli.subprocess, "run", fake_run) # Prevent run_gateway from actually starting the gateway - def fake_start_gateway(**kwargs): - import asyncio - f = asyncio.Future() - f.set_result(True) - return f + async def fake_start_gateway(**kwargs): + return True monkeypatch.setattr("gateway.run.start_gateway", fake_start_gateway) @@ -163,7 +185,16 @@ class TestRequireServiceInstalled: class TestGeneratedSystemdUnits: - def test_user_unit_avoids_recursive_execstop_and_uses_extended_stop_timeout(self): + def _expected_timeout_stop_sec(self) -> str: + timeout = int(max(60, DEFAULT_GATEWAY_RESTART_DRAIN_TIMEOUT) + 30) + return f"TimeoutStopSec={timeout}" + + def test_user_unit_avoids_recursive_execstop_and_uses_extended_stop_timeout(self, monkeypatch): + monkeypatch.setattr( + gateway_cli, + "_get_restart_drain_timeout", + lambda: DEFAULT_GATEWAY_RESTART_DRAIN_TIMEOUT, + ) unit = gateway_cli.generate_systemd_unit(system=False) assert "ExecStart=" in unit @@ -173,7 +204,7 @@ class TestGeneratedSystemdUnits: # TimeoutStopSec must exceed the default drain_timeout (60s) so # systemd doesn't SIGKILL the cgroup before post-interrupt cleanup # (tool subprocess kill, adapter disconnect) runs — issue #8202. - assert "TimeoutStopSec=90" in unit + assert self._expected_timeout_stop_sec() in unit def test_user_unit_includes_resolved_node_directory_in_path(self, monkeypatch): monkeypatch.setattr(gateway_cli.shutil, "which", lambda cmd: "/home/test/.nvm/versions/node/v24.14.0/bin/node" if cmd == "node" else None) @@ -219,7 +250,12 @@ class TestGeneratedSystemdUnits: assert "/mnt/c/WINDOWS/system32" in unit - def test_system_unit_avoids_recursive_execstop_and_uses_extended_stop_timeout(self): + def test_system_unit_avoids_recursive_execstop_and_uses_extended_stop_timeout(self, monkeypatch): + monkeypatch.setattr( + gateway_cli, + "_get_restart_drain_timeout", + lambda: DEFAULT_GATEWAY_RESTART_DRAIN_TIMEOUT, + ) unit = gateway_cli.generate_systemd_unit(system=True) assert "ExecStart=" in unit @@ -229,7 +265,7 @@ class TestGeneratedSystemdUnits: # TimeoutStopSec must exceed the default drain_timeout (60s) so # systemd doesn't SIGKILL the cgroup before post-interrupt cleanup # (tool subprocess kill, adapter disconnect) runs — issue #8202. - assert "TimeoutStopSec=90" in unit + assert self._expected_timeout_stop_sec() in unit assert "WantedBy=multi-user.target" in unit From b816fd4e26d6c7260814f53d5ba7c7eb065548c7 Mon Sep 17 00:00:00 2001 From: helix4u <4317663+helix4u@users.noreply.github.com> Date: Mon, 4 May 2026 16:31:20 -0600 Subject: [PATCH 10/21] fix(tui): complete absolute paths as paths --- ui-tui/src/__tests__/useCompletion.test.ts | 35 +++++++++++++ ui-tui/src/hooks/useCompletion.ts | 57 ++++++++++++++-------- 2 files changed, 71 insertions(+), 21 deletions(-) create mode 100644 ui-tui/src/__tests__/useCompletion.test.ts diff --git a/ui-tui/src/__tests__/useCompletion.test.ts b/ui-tui/src/__tests__/useCompletion.test.ts new file mode 100644 index 0000000000..67a9fcfea8 --- /dev/null +++ b/ui-tui/src/__tests__/useCompletion.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, it } from 'vitest' + +import { completionRequestForInput } from '../hooks/useCompletion.js' + +describe('completionRequestForInput', () => { + it('routes real slash commands to slash completion', () => { + expect(completionRequestForInput('/help')).toMatchObject({ + method: 'complete.slash', + params: { text: '/help' }, + replaceFrom: 1 + }) + }) + + it('does not route absolute paths through slash completion', () => { + expect( + completionRequestForInput('/home/d/Desktop/agenda/CrimsonRed/.hermes/plans/2026-05-04-HANDOFF-NEXT.md') + ).toMatchObject({ + method: 'complete.path', + params: { word: '/home/d/Desktop/agenda/CrimsonRed/.hermes/plans/2026-05-04-HANDOFF-NEXT.md' }, + replaceFrom: 0 + }) + }) + + it('keeps path completion for trailing absolute path tokens', () => { + expect(completionRequestForInput('read /home/d/Desktop/file.md')).toMatchObject({ + method: 'complete.path', + params: { word: '/home/d/Desktop/file.md' }, + replaceFrom: 5 + }) + }) + + it('leaves plain text alone', () => { + expect(completionRequestForInput('hello there')).toBeNull() + }) +}) diff --git a/ui-tui/src/hooks/useCompletion.ts b/ui-tui/src/hooks/useCompletion.ts index 08bd4945d7..6bafc35843 100644 --- a/ui-tui/src/hooks/useCompletion.ts +++ b/ui-tui/src/hooks/useCompletion.ts @@ -1,12 +1,43 @@ import { useEffect, useRef, useState } from 'react' import type { CompletionItem } from '../app/interfaces.js' +import { looksLikeSlashCommand } from '../domain/slash.js' import type { GatewayClient } from '../gatewayClient.js' import type { CompletionResponse } from '../gatewayTypes.js' import { asRpcResult } from '../lib/rpc.js' const TAB_PATH_RE = /((?:["']?(?:[A-Za-z]:[\\/]|\.{1,2}\/|~\/|\/|@|[^"'`\s]+\/))[^\s]*)$/ +export function completionRequestForInput( + input: string +): + | { method: 'complete.path'; params: { word: string }; replaceFrom: number } + | { method: 'complete.slash'; params: { text: string }; replaceFrom: number } + | null { + const isSlashCommand = looksLikeSlashCommand(input) + const pathWord = isSlashCommand ? null : (input.match(TAB_PATH_RE)?.[1] ?? null) + + if (!isSlashCommand && !pathWord) { + return null + } + + // `/model` / `/provider` use the two-step ModelPicker (real curated IDs). + // Slash completion here only showed short aliases + vendor/family meta. + if (isSlashCommand && /^\/(?:model|provider)(?:\s|$)/.test(input)) { + return null + } + + if (isSlashCommand) { + return { method: 'complete.slash', params: { text: input }, replaceFrom: 1 } + } + + return { + method: 'complete.path', + params: { word: pathWord! }, + replaceFrom: input.length - pathWord!.length + } +} + export function useCompletion(input: string, blocked: boolean, gw: GatewayClient) { const [completions, setCompletions] = useState([]) const [compIdx, setCompIdx] = useState(0) @@ -33,35 +64,19 @@ export function useCompletion(input: string, blocked: boolean, gw: GatewayClient ref.current = input - const isSlash = input.startsWith('/') - const pathWord = isSlash ? null : (input.match(TAB_PATH_RE)?.[1] ?? null) - - if (!isSlash && !pathWord) { + const request = completionRequestForInput(input) + if (!request) { clear() return } - // `/model` / `/provider` use the two-step ModelPicker (real curated IDs). - // Slash completion here only showed short aliases + vendor/family meta. - if (isSlash && /^\/(?:model|provider)(?:\s|$)/.test(input)) { - clear() - - return - } - - const pathReplace = input.length - (pathWord?.length ?? 0) - const t = setTimeout(() => { if (ref.current !== input) { return } - const req = isSlash - ? gw.request('complete.slash', { text: input }) - : gw.request('complete.path', { word: pathWord }) - - req + gw.request(request.method, request.params) .then(raw => { if (ref.current !== input) { return @@ -71,7 +86,7 @@ export function useCompletion(input: string, blocked: boolean, gw: GatewayClient setCompletions(r?.items ?? []) setCompIdx(0) - setCompReplace(isSlash ? (r?.replace_from ?? 1) : pathReplace) + setCompReplace(request.method === 'complete.slash' ? (r?.replace_from ?? 1) : request.replaceFrom) }) .catch((e: unknown) => { if (ref.current !== input) { @@ -86,7 +101,7 @@ export function useCompletion(input: string, blocked: boolean, gw: GatewayClient } ]) setCompIdx(0) - setCompReplace(isSlash ? 1 : pathReplace) + setCompReplace(request.replaceFrom) }) }, 60) From a1bed18194ff1ee8de1bf3e81007ddba06b61042 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Mon, 4 May 2026 20:09:31 -0700 Subject: [PATCH 11/21] docs: clarify that the Docker terminal backend is a single persistent container (#20003) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The docs were ambiguous about whether the Docker terminal backend spins up a fresh container per command or reuses a long-lived one. It's the latter — Hermes starts one container on first use and routes every terminal, file, and execute_code call through docker exec into that same container for the life of the process (across /new, /reset, and delegate_task subagents). Working-directory changes, installed packages, and files in /workspace persist from one tool call to the next, like a local shell. - configuration.md: lead the Docker Backend section with the persistence model before the YAML example; sharpen the Backend Overview table row. - features/tools.md: expand the Docker Backend block (previously just a 2-line YAML stub) with a clear statement of the persistent-container semantics and a pointer to the full lifecycle section. - docker.md: tighten the 'Docker as a terminal backend' bullet and the 'Skills and credential files' paragraph to call out the single-container model explicitly. --- website/docs/user-guide/configuration.md | 4 +++- website/docs/user-guide/docker.md | 4 ++-- website/docs/user-guide/features/tools.md | 4 ++++ 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/website/docs/user-guide/configuration.md b/website/docs/user-guide/configuration.md index 517cb2e988..4c5e51860e 100644 --- a/website/docs/user-guide/configuration.md +++ b/website/docs/user-guide/configuration.md @@ -103,7 +103,7 @@ For cloud sandboxes such as Modal, Daytona, and Vercel Sandbox, `container_persi | Backend | Where commands run | Isolation | Best for | |---------|-------------------|-----------|----------| | **local** | Your machine directly | None | Development, personal use | -| **docker** | Docker container | Full (namespaces, cap-drop) | Safe sandboxing, CI/CD | +| **docker** | Single persistent Docker container (shared across session, `/new`, subagents) | Full (namespaces, cap-drop) | Safe sandboxing, CI/CD | | **ssh** | Remote server via SSH | Network boundary | Remote dev, powerful hardware | | **modal** | Modal cloud sandbox | Full (cloud VM) | Ephemeral cloud compute, evals | | **daytona** | Daytona workspace | Full (cloud container) | Managed cloud dev environments | @@ -127,6 +127,8 @@ The agent has the same filesystem access as your user account. Use `hermes tools Runs commands inside a Docker container with security hardening (all capabilities dropped, no privilege escalation, PID limits). +**Single persistent container, not per-command.** Hermes starts ONE long-lived container on first use and routes every terminal, file, and `execute_code` call through `docker exec` into that same container — across sessions, `/new`, `/reset`, and `delegate_task` subagents — for the lifetime of the Hermes process. Working-directory changes, installed packages, and files in `/workspace` carry over from one tool call to the next, just like a local shell. The container is stopped and removed on shutdown. See **Container lifecycle** below for details. + ```yaml terminal: backend: docker diff --git a/website/docs/user-guide/docker.md b/website/docs/user-guide/docker.md index 2a13fe6662..e99459cc1d 100644 --- a/website/docs/user-guide/docker.md +++ b/website/docs/user-guide/docker.md @@ -9,7 +9,7 @@ description: "Running Hermes Agent in Docker and using Docker as a terminal back There are two distinct ways Docker intersects with Hermes Agent: 1. **Running Hermes IN Docker** — the agent itself runs inside a container (this page's primary focus) -2. **Docker as a terminal backend** — the agent runs on your host but executes commands inside a Docker sandbox (see [Configuration → terminal.backend](./configuration.md)) +2. **Docker as a terminal backend** — the agent runs on your host but executes every command inside a single, persistent Docker sandbox container that survives across tool calls, `/new`, and subagents for the life of the Hermes process (see [Configuration → Docker Backend](./configuration.md#docker-backend)) This page covers option 1. The container stores all user data (config, API keys, sessions, skills, memories) in a single directory mounted from the host at `/opt/data`. The image itself is stateless and can be upgraded by pulling a new version without losing any configuration. @@ -279,7 +279,7 @@ docker compose up -d ## Skills and credential files -When using Docker as the execution environment (not the methods above, but when the agent runs commands inside a Docker sandbox), Hermes automatically bind-mounts the skills directory (`~/.hermes/skills/`) and any credential files declared by skills into the container as read-only volumes. This means skill scripts, templates, and references are available inside the sandbox without manual configuration. +When using Docker as the execution environment (not the methods above, but when the agent runs commands inside a Docker sandbox — see [Configuration → Docker Backend](./configuration.md#docker-backend)), Hermes reuses a single long-lived container for all tool calls and automatically bind-mounts the skills directory (`~/.hermes/skills/`) and any credential files declared by skills into that container as read-only volumes. Skill scripts, templates, and references are available inside the sandbox without manual configuration, and because the container persists for the life of the Hermes process, any dependencies you install or files you write stay around for the next tool call. The same syncing happens for SSH and Modal backends — skills and credential files are uploaded via rsync or the Modal mount API before each command. diff --git a/website/docs/user-guide/features/tools.md b/website/docs/user-guide/features/tools.md index 80e1800812..9f9eddbb51 100644 --- a/website/docs/user-guide/features/tools.md +++ b/website/docs/user-guide/features/tools.md @@ -84,6 +84,10 @@ terminal: docker_image: python:3.11-slim ``` +**One persistent container, shared across the whole process.** Hermes starts a single long-lived container on first use (`docker run -d ... sleep 2h`) and routes every terminal, file, and `execute_code` call through `docker exec` into that same container. Working-directory changes, installed packages, environment tweaks, and files written to `/workspace` all carry over from one tool call to the next, across `/new`, `/reset`, and `delegate_task` subagents, for the lifetime of the Hermes process. The container is stopped and removed on shutdown. + +This means the Docker backend behaves like a persistent sandbox VM, not a fresh container per command. If you `pip install foo` once, it's there for the rest of the session. If you `cd /workspace/project`, subsequent `ls` calls see that directory. See [Configuration → Docker Backend](../configuration.md#docker-backend) for the full lifecycle details and the `container_persistent` flag that controls whether `/workspace` and `/root` survive across Hermes restarts. + ### SSH Backend Recommended for security — agent can't modify its own code: From f7918c934927bccb7b1c5780d813100076b64998 Mon Sep 17 00:00:00 2001 From: Stephen Schoettler Date: Thu, 30 Apr 2026 20:15:02 -0700 Subject: [PATCH 12/21] test(teams): mock ClientOptions in adapter tests --- tests/gateway/test_teams.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/gateway/test_teams.py b/tests/gateway/test_teams.py index 2befceec94..28442788a5 100644 --- a/tests/gateway/test_teams.py +++ b/tests/gateway/test_teams.py @@ -32,6 +32,9 @@ def _ensure_teams_mock(): microsoft_teams_api_activities_invoke_adaptive_card = types.ModuleType( "microsoft_teams.api.activities.invoke.adaptive_card" ) + microsoft_teams_common = types.ModuleType("microsoft_teams.common") + microsoft_teams_common_http = types.ModuleType("microsoft_teams.common.http") + microsoft_teams_common_http_client = types.ModuleType("microsoft_teams.common.http.client") microsoft_teams_api_models = types.ModuleType("microsoft_teams.api.models") microsoft_teams_api_models_adaptive_card = types.ModuleType("microsoft_teams.api.models.adaptive_card") microsoft_teams_api_models_invoke_response = types.ModuleType("microsoft_teams.api.models.invoke_response") @@ -76,6 +79,7 @@ def _ensure_teams_mock(): microsoft_teams_apps.App = MockApp microsoft_teams_apps.ActivityContext = MagicMock + microsoft_teams_common_http_client.ClientOptions = MagicMock # MessageActivity mock microsoft_teams_api.MessageActivity = MagicMock @@ -143,6 +147,9 @@ def _ensure_teams_mock(): "microsoft_teams.api.activities.typing": microsoft_teams_api_activities_typing, "microsoft_teams.api.activities.invoke": microsoft_teams_api_activities_invoke, "microsoft_teams.api.activities.invoke.adaptive_card": microsoft_teams_api_activities_invoke_adaptive_card, + "microsoft_teams.common": microsoft_teams_common, + "microsoft_teams.common.http": microsoft_teams_common_http, + "microsoft_teams.common.http.client": microsoft_teams_common_http_client, "microsoft_teams.api.models": microsoft_teams_api_models, "microsoft_teams.api.models.adaptive_card": microsoft_teams_api_models_adaptive_card, "microsoft_teams.api.models.invoke_response": microsoft_teams_api_models_invoke_response, From 1d938832a7dfd2cd661d50017ff839d599a820db Mon Sep 17 00:00:00 2001 From: Stephen Schoettler Date: Thu, 30 Apr 2026 20:26:54 -0700 Subject: [PATCH 13/21] test(kanban): patch dashboard websocket token stub --- tests/plugins/test_kanban_dashboard_plugin.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/plugins/test_kanban_dashboard_plugin.py b/tests/plugins/test_kanban_dashboard_plugin.py index 23589ce690..32e40ceb4e 100644 --- a/tests/plugins/test_kanban_dashboard_plugin.py +++ b/tests/plugins/test_kanban_dashboard_plugin.py @@ -505,9 +505,11 @@ def test_ws_events_rejects_when_token_required(tmp_path, monkeypatch): kb.init_db() # Stub web_server so _check_ws_token has a token to compare against. + import hermes_cli import types stub = types.SimpleNamespace(_SESSION_TOKEN="secret-xyz") monkeypatch.setitem(sys.modules, "hermes_cli.web_server", stub) + monkeypatch.setattr(hermes_cli, "web_server", stub, raising=False) app = FastAPI() app.include_router(_load_plugin_router(), prefix="/api/plugins/kanban") From c77a6e3faaf3d400f639213ba3af02a3891ad05a Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Mon, 4 May 2026 20:58:21 -0700 Subject: [PATCH 14/21] chore(security): add OSV-Scanner CI + Dependabot for github-actions only (#20037) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds two supply-chain controls that complement our existing pinning strategy (full-SHA action pins, exact-version source dep pins via uv.lock / package-lock.json) without undermining it. .github/workflows/osv-scanner.yml Detection-only scan of uv.lock and the ui-tui/website package-locks against the OSV vulnerability database. Runs on PRs that touch lockfiles, on push to main, and weekly against main so CVEs published after merge still surface. Uses Google's officially- recommended reusable workflow pinned by full SHA (v2.3.5). Findings upload to the Security tab; fail-on-vuln is disabled so pre-existing vulns in pinned deps do not block merges — we move pins deliberately, not under CI pressure. .github/dependabot.yml Scoped to github-actions only. Action pins must be moved when upstream publishes patches (often themselves security fixes); Dependabot opens a PR with the new SHA + release notes for normal review. Source-dependency ecosystems (pip, npm) are deliberately NOT enabled — automatic version-bump PRs against uv.lock / package-lock.json would fight our pinning strategy. CVE-driven security updates for source deps are enabled separately via the repo's Dependabot security updates setting (GitHub UI), which fires only when a pinned version becomes known-vulnerable. --- .github/dependabot.yml | 44 ++++++++++++++++++++ .github/workflows/osv-scanner.yml | 67 +++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+) create mode 100644 .github/dependabot.yml create mode 100644 .github/workflows/osv-scanner.yml diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000000..3854c8f930 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,44 @@ +# Dependabot configuration for hermes-agent. +# +# Deliberately scoped to github-actions only. +# +# We do NOT enable Dependabot for pip / npm / any source-dependency ecosystem +# because we pin source dependencies exactly (uv.lock, package-lock.json) as +# part of our supply-chain posture. Automatic version-bump PRs against those +# pins would undermine the strategy — pins are moved deliberately, after +# review, not on a schedule. +# +# github-actions is the exception: action pins (we use full commit SHAs per +# supply-chain policy) must be updated when upstream actions publish +# patches — usually themselves security fixes. Dependabot opens a PR with +# the new SHA and release notes; we review and merge like any other PR. +# +# Security-update PRs for source dependencies (opened ONLY when a CVE is +# published affecting a currently-pinned version) are enabled separately +# via the repo's Dependabot security updates setting +# (Settings → Code security → Dependabot → Dependabot security updates). +# Those are CVE-only, not schedule-driven, and do not conflict with our +# pinning strategy — they fire when a pinned version becomes known-bad, +# which is exactly when we want to move the pin. + +version: 2 +updates: + - package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "weekly" + day: "monday" + open-pull-requests-limit: 5 + labels: + - "dependencies" + - "github-actions" + commit-message: + prefix: "chore(actions)" + include: "scope" + groups: + # Batch routine action bumps into one PR per week to reduce noise. + # Security updates still open individually and bypass grouping. + actions-minor-patch: + update-types: + - "minor" + - "patch" diff --git a/.github/workflows/osv-scanner.yml b/.github/workflows/osv-scanner.yml new file mode 100644 index 0000000000..db8c3d75ce --- /dev/null +++ b/.github/workflows/osv-scanner.yml @@ -0,0 +1,67 @@ +name: OSV-Scanner + +# Scans lockfiles (uv.lock, package-lock.json) against the OSV vulnerability +# database. Runs on every PR that touches a lockfile and on a weekly schedule +# against main. +# +# This is detection-only — OSV-Scanner does NOT open PRs or modify pins. +# It reports known CVEs in currently-pinned dependency versions so we can +# decide when and how to patch on our own schedule. Our pinning strategy +# (full SHA / exact version) is preserved; only the notification signal +# is added. +# +# Complements the existing supply-chain-audit.yml workflow (which scans +# for malicious code patterns in PR diffs) by covering the orthogonal +# "currently-pinned dep became known-vulnerable" case. +# +# Uses Google's officially-recommended reusable workflow, pinned by SHA. +# Findings land in the repo's Security tab (Code Scanning > OSV-Scanner). +# fail-on-vuln is disabled so the job does not block merges on pre-existing +# vulnerabilities in pinned deps that we may need to patch deliberately. + +on: + pull_request: + branches: [main] + paths: + - 'uv.lock' + - 'pyproject.toml' + - 'package.json' + - 'package-lock.json' + - 'ui-tui/package.json' + - 'ui-tui/package-lock.json' + - 'website/package.json' + - 'website/package-lock.json' + - '.github/workflows/osv-scanner.yml' + push: + branches: [main] + paths: + - 'uv.lock' + - 'pyproject.toml' + - 'package.json' + - 'package-lock.json' + - 'ui-tui/package-lock.json' + - 'website/package-lock.json' + schedule: + # Weekly scan against main — catches CVEs published after merge for + # deps that haven't changed since. + - cron: '0 9 * * 1' + workflow_dispatch: + +permissions: + # Required by the reusable workflow to upload SARIF to the Security tab. + actions: read + contents: read + security-events: write + +jobs: + scan: + name: Scan lockfiles + uses: google/osv-scanner-action/.github/workflows/osv-scanner-reusable.yml@c51854704019a247608d928f370c98740469d4b5 # v2.3.5 + with: + # Scan explicit lockfiles rather than recursing, so we only look at + # the three sources of truth and skip vendored / test / worktree dirs. + scan-args: |- + --lockfile=uv.lock + --lockfile=ui-tui/package-lock.json + --lockfile=website/package-lock.json + fail-on-vuln: false From ef94aa201fba8c1c95a1c8ee9a4d1ee66ca40dd1 Mon Sep 17 00:00:00 2001 From: Aamir Jawaid Date: Fri, 1 May 2026 04:30:41 +0000 Subject: [PATCH 15/21] docs(teams): add Teams to sidebar Co-Authored-By: Claude Sonnet 4.6 --- website/sidebars.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/website/sidebars.ts b/website/sidebars.ts index c30fec6c52..a24366474e 100644 --- a/website/sidebars.ts +++ b/website/sidebars.ts @@ -133,6 +133,7 @@ const sidebars: SidebarsConfig = { 'user-guide/messaging/bluebubbles', 'user-guide/messaging/qqbot', 'user-guide/messaging/yuanbao', + 'user-guide/messaging/teams', 'user-guide/messaging/open-webui', 'user-guide/messaging/webhooks', ], From 93869b48ab520ad8df386e2057013c5f78bb8d7a Mon Sep 17 00:00:00 2001 From: Aamir Jawaid Date: Fri, 1 May 2026 04:39:44 +0000 Subject: [PATCH 16/21] docs: add Microsoft Teams to platform lists across docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Update all platform enumeration lists to include Teams: index.md, quickstart.md, integrations/index.md, sessions.md, slash-commands.md, updating.md, hooks.md, hermes-agent skill. Skipped PII redaction docs — Teams uses AAD object IDs, not phone numbers, so redaction doesn't apply there. Co-Authored-By: Claude Sonnet 4.6 --- website/docs/getting-started/quickstart.md | 4 ++-- website/docs/getting-started/updating.md | 2 +- website/docs/index.md | 4 ++-- website/docs/integrations/index.md | 2 +- website/docs/reference/slash-commands.md | 2 +- website/docs/user-guide/features/hooks.md | 4 ++-- website/docs/user-guide/sessions.md | 2 +- .../autonomous-ai-agents/autonomous-ai-agents-hermes-agent.md | 2 +- 8 files changed, 11 insertions(+), 11 deletions(-) diff --git a/website/docs/getting-started/quickstart.md b/website/docs/getting-started/quickstart.md index c9af170420..01c5239a11 100644 --- a/website/docs/getting-started/quickstart.md +++ b/website/docs/getting-started/quickstart.md @@ -204,7 +204,7 @@ Only after the base chat works. Pick what you need: hermes gateway setup # Interactive platform configuration ``` -Connect [Telegram](/docs/user-guide/messaging/telegram), [Discord](/docs/user-guide/messaging/discord), [Slack](/docs/user-guide/messaging/slack), [WhatsApp](/docs/user-guide/messaging/whatsapp), [Signal](/docs/user-guide/messaging/signal), [Email](/docs/user-guide/messaging/email), or [Home Assistant](/docs/user-guide/messaging/homeassistant). +Connect [Telegram](/docs/user-guide/messaging/telegram), [Discord](/docs/user-guide/messaging/discord), [Slack](/docs/user-guide/messaging/slack), [WhatsApp](/docs/user-guide/messaging/whatsapp), [Signal](/docs/user-guide/messaging/signal), [Email](/docs/user-guide/messaging/email), or [Home Assistant](/docs/user-guide/messaging/homeassistant), or [Microsoft Teams](/docs/user-guide/messaging/teams). ### Automation and tools @@ -307,7 +307,7 @@ That sequence gets you from "broken vibes" back to a known state fast. - **[CLI Guide](../user-guide/cli.md)** — Master the terminal interface - **[Configuration](../user-guide/configuration.md)** — Customize your setup -- **[Messaging Gateway](../user-guide/messaging/index.md)** — Connect Telegram, Discord, Slack, WhatsApp, Signal, Email, or Home Assistant +- **[Messaging Gateway](../user-guide/messaging/index.md)** — Connect Telegram, Discord, Slack, WhatsApp, Signal, Email, Home Assistant, Teams, and more - **[Tools & Toolsets](../user-guide/features/tools.md)** — Explore available capabilities - **[AI Providers](../integrations/providers.md)** — Full provider list and setup details - **[Skills System](../user-guide/features/skills.md)** — Reusable workflows and knowledge diff --git a/website/docs/getting-started/updating.md b/website/docs/getting-started/updating.md index 8bd84ba157..c39363a9e0 100644 --- a/website/docs/getting-started/updating.md +++ b/website/docs/getting-started/updating.md @@ -107,7 +107,7 @@ Compare against the latest release at the [GitHub releases page](https://github. ### Updating from Messaging Platforms -You can also update directly from Telegram, Discord, Slack, or WhatsApp by sending: +You can also update directly from Telegram, Discord, Slack, WhatsApp, or Teams by sending: ``` /update diff --git a/website/docs/index.md b/website/docs/index.md index 17a2ac8cc2..db7106d955 100644 --- a/website/docs/index.md +++ b/website/docs/index.md @@ -28,7 +28,7 @@ It's not a coding copilot tethered to an IDE or a chatbot wrapper around a singl | 📖 **[Quickstart Tutorial](/docs/getting-started/quickstart)** | Your first conversation and key features to try | | 🗺️ **[Learning Path](/docs/getting-started/learning-path)** | Find the right docs for your experience level | | ⚙️ **[Configuration](/docs/user-guide/configuration)** | Config file, providers, models, and options | -| 💬 **[Messaging Gateway](/docs/user-guide/messaging)** | Set up Telegram, Discord, Slack, or WhatsApp | +| 💬 **[Messaging Gateway](/docs/user-guide/messaging)** | Set up Telegram, Discord, Slack, WhatsApp, Teams, or more | | 🔧 **[Tools & Toolsets](/docs/user-guide/features/tools)** | 68 built-in tools and how to configure them | | 🧠 **[Memory System](/docs/user-guide/features/memory)** | Persistent memory that grows across sessions | | 📚 **[Skills System](/docs/user-guide/features/skills)** | Procedural memory the agent creates and reuses | @@ -47,7 +47,7 @@ It's not a coding copilot tethered to an IDE or a chatbot wrapper around a singl - **A closed learning loop** — Agent-curated memory with periodic nudges, autonomous skill creation, skill self-improvement during use, FTS5 cross-session recall with LLM summarization, and [Honcho](https://github.com/plastic-labs/honcho) dialectic user modeling - **Runs anywhere, not just your laptop** — 6 terminal backends: local, Docker, SSH, Daytona, Singularity, Modal. Daytona and Modal offer serverless persistence — your environment hibernates when idle, costing nearly nothing -- **Lives where you do** — CLI, Telegram, Discord, Slack, WhatsApp, Signal, Matrix, Mattermost, Email, SMS, DingTalk, Feishu, WeCom, BlueBubbles, Home Assistant — 15+ platforms from one gateway +- **Lives where you do** — CLI, Telegram, Discord, Slack, WhatsApp, Signal, Matrix, Mattermost, Email, SMS, DingTalk, Feishu, WeCom, BlueBubbles, Home Assistant, Microsoft Teams — 15+ platforms from one gateway - **Built by model trainers** — Created by [Nous Research](https://nousresearch.com), the lab behind Hermes, Nomos, and Psyche. Works with [Nous Portal](https://portal.nousresearch.com), [OpenRouter](https://openrouter.ai), OpenAI, or any endpoint - **Scheduled automations** — Built-in cron with delivery to any platform - **Delegates & parallelizes** — Spawn isolated subagents for parallel workstreams. Programmatic Tool Calling via `execute_code` collapses multi-step pipelines into single inference calls diff --git a/website/docs/integrations/index.md b/website/docs/integrations/index.md index f511e03bf5..20b86565d8 100644 --- a/website/docs/integrations/index.md +++ b/website/docs/integrations/index.md @@ -82,7 +82,7 @@ Speech-to-text supports six providers: local faster-whisper (free, runs on-devic Hermes runs as a gateway bot on 15+ messaging platforms, all configured through the same `gateway` subsystem: -- **[Telegram](/docs/user-guide/messaging/telegram)**, **[Discord](/docs/user-guide/messaging/discord)**, **[Slack](/docs/user-guide/messaging/slack)**, **[WhatsApp](/docs/user-guide/messaging/whatsapp)**, **[Signal](/docs/user-guide/messaging/signal)**, **[Matrix](/docs/user-guide/messaging/matrix)**, **[Mattermost](/docs/user-guide/messaging/mattermost)**, **[Email](/docs/user-guide/messaging/email)**, **[SMS](/docs/user-guide/messaging/sms)**, **[DingTalk](/docs/user-guide/messaging/dingtalk)**, **[Feishu/Lark](/docs/user-guide/messaging/feishu)**, **[WeCom](/docs/user-guide/messaging/wecom)**, **[WeCom Callback](/docs/user-guide/messaging/wecom-callback)**, **[Weixin](/docs/user-guide/messaging/weixin)**, **[BlueBubbles](/docs/user-guide/messaging/bluebubbles)**, **[QQ Bot](/docs/user-guide/messaging/qqbot)**, **[Home Assistant](/docs/user-guide/messaging/homeassistant)**, **[Webhooks](/docs/user-guide/messaging/webhooks)** +- **[Telegram](/docs/user-guide/messaging/telegram)**, **[Discord](/docs/user-guide/messaging/discord)**, **[Slack](/docs/user-guide/messaging/slack)**, **[WhatsApp](/docs/user-guide/messaging/whatsapp)**, **[Signal](/docs/user-guide/messaging/signal)**, **[Matrix](/docs/user-guide/messaging/matrix)**, **[Mattermost](/docs/user-guide/messaging/mattermost)**, **[Email](/docs/user-guide/messaging/email)**, **[SMS](/docs/user-guide/messaging/sms)**, **[DingTalk](/docs/user-guide/messaging/dingtalk)**, **[Feishu/Lark](/docs/user-guide/messaging/feishu)**, **[WeCom](/docs/user-guide/messaging/wecom)**, **[WeCom Callback](/docs/user-guide/messaging/wecom-callback)**, **[Weixin](/docs/user-guide/messaging/weixin)**, **[BlueBubbles](/docs/user-guide/messaging/bluebubbles)**, **[QQ Bot](/docs/user-guide/messaging/qqbot)**, **[Home Assistant](/docs/user-guide/messaging/homeassistant)**, **[Microsoft Teams](/docs/user-guide/messaging/teams)**, **[Webhooks](/docs/user-guide/messaging/webhooks)** See the [Messaging Gateway overview](/docs/user-guide/messaging) for the platform comparison table and setup guide. diff --git a/website/docs/reference/slash-commands.md b/website/docs/reference/slash-commands.md index 75158d6a0d..1348062af1 100644 --- a/website/docs/reference/slash-commands.md +++ b/website/docs/reference/slash-commands.md @@ -130,7 +130,7 @@ Commands support prefix matching: typing `/h` resolves to `/help`, `/mod` resolv ## Messaging slash commands -The messaging gateway supports the following built-in commands inside Telegram, Discord, Slack, WhatsApp, Signal, Email, and Home Assistant chats: +The messaging gateway supports the following built-in commands inside Telegram, Discord, Slack, WhatsApp, Signal, Email, Home Assistant, and Teams chats: | Command | Description | |---------|-------------| diff --git a/website/docs/user-guide/features/hooks.md b/website/docs/user-guide/features/hooks.md index e3893c0a23..92e9bfefc1 100644 --- a/website/docs/user-guide/features/hooks.md +++ b/website/docs/user-guide/features/hooks.md @@ -18,7 +18,7 @@ All three systems are non-blocking — errors in any hook are caught and logged, ## Gateway Event Hooks -Gateway hooks fire automatically during gateway operation (Telegram, Discord, Slack, WhatsApp) without blocking the main agent pipeline. +Gateway hooks fire automatically during gateway operation (Telegram, Discord, Slack, WhatsApp, Teams) without blocking the main agent pipeline. ### Creating a Hook @@ -346,7 +346,7 @@ An earlier version of Hermes shipped this as a built-in hook and silently spawne 5. Errors in any handler are caught and logged — a broken hook never crashes the agent :::info -Gateway hooks only fire in the **gateway** (Telegram, Discord, Slack, WhatsApp). The CLI does not load gateway hooks. For hooks that work everywhere, use [plugin hooks](#plugin-hooks). +Gateway hooks only fire in the **gateway** (Telegram, Discord, Slack, WhatsApp, Teams). The CLI does not load gateway hooks. For hooks that work everywhere, use [plugin hooks](#plugin-hooks). ::: ## Plugin Hooks diff --git a/website/docs/user-guide/sessions.md b/website/docs/user-guide/sessions.md index ec101f6b45..5094edf64c 100644 --- a/website/docs/user-guide/sessions.md +++ b/website/docs/user-guide/sessions.md @@ -10,7 +10,7 @@ Hermes Agent automatically saves every conversation as a session. Sessions enabl ## How Sessions Work -Every conversation — whether from the CLI, Telegram, Discord, Slack, WhatsApp, Signal, Matrix, or any other messaging platform — is stored as a session with full message history. Sessions are tracked in two complementary systems: +Every conversation — whether from the CLI, Telegram, Discord, Slack, WhatsApp, Signal, Matrix, Teams, or any other messaging platform — is stored as a session with full message history. Sessions are tracked in two complementary systems: 1. **SQLite database** (`~/.hermes/state.db`) — structured session metadata with FTS5 full-text search 2. **JSONL transcripts** (`~/.hermes/sessions/`) — raw conversation transcripts including tool calls (gateway) diff --git a/website/docs/user-guide/skills/bundled/autonomous-ai-agents/autonomous-ai-agents-hermes-agent.md b/website/docs/user-guide/skills/bundled/autonomous-ai-agents/autonomous-ai-agents-hermes-agent.md index 1159c64398..c1c501932c 100644 --- a/website/docs/user-guide/skills/bundled/autonomous-ai-agents/autonomous-ai-agents-hermes-agent.md +++ b/website/docs/user-guide/skills/bundled/autonomous-ai-agents/autonomous-ai-agents-hermes-agent.md @@ -165,7 +165,7 @@ hermes gateway status Check status hermes gateway setup Configure platforms ``` -Supported platforms: Telegram, Discord, Slack, WhatsApp, Signal, Email, SMS, Matrix, Mattermost, Home Assistant, DingTalk, Feishu, WeCom, BlueBubbles (iMessage), Weixin (WeChat), API Server, Webhooks. Open WebUI connects via the API Server adapter. +Supported platforms: Telegram, Discord, Slack, WhatsApp, Signal, Email, SMS, Matrix, Mattermost, Home Assistant, DingTalk, Feishu, WeCom, BlueBubbles (iMessage), Weixin (WeChat), Microsoft Teams, API Server, Webhooks. Open WebUI connects via the API Server adapter. Platform docs: https://hermes-agent.nousresearch.com/docs/user-guide/messaging/ From 10f89d7b724bb9f6ae8e97dafbed6b8dba01856e Mon Sep 17 00:00:00 2001 From: Aamir Jawaid Date: Fri, 1 May 2026 04:44:45 +0000 Subject: [PATCH 17/21] docs(teams): add Teams to messaging/index.md - Add to platform description and intro paragraph - Add row to platform comparison table (images + typing) - Add node to architecture mermaid diagram - Add TEAMS_ALLOWED_USERS to security examples - Add to platform-specific toolsets table - Add to Next Steps links Co-Authored-By: Claude Sonnet 4.6 --- website/docs/user-guide/messaging/index.md | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/website/docs/user-guide/messaging/index.md b/website/docs/user-guide/messaging/index.md index 578a826b64..07342e8cf8 100644 --- a/website/docs/user-guide/messaging/index.md +++ b/website/docs/user-guide/messaging/index.md @@ -1,12 +1,12 @@ --- sidebar_position: 1 title: "Messaging Gateway" -description: "Chat with Hermes from Telegram, Discord, Slack, WhatsApp, Signal, SMS, Email, Home Assistant, Mattermost, Matrix, DingTalk, Yuanbao, Webhooks, or any OpenAI-compatible frontend via the API server — architecture and setup overview" +description: "Chat with Hermes from Telegram, Discord, Slack, WhatsApp, Signal, SMS, Email, Home Assistant, Mattermost, Matrix, DingTalk, Yuanbao, Microsoft Teams, Webhooks, or any OpenAI-compatible frontend via the API server — architecture and setup overview" --- # Messaging Gateway -Chat with Hermes from Telegram, Discord, Slack, WhatsApp, Signal, SMS, Email, Home Assistant, Mattermost, Matrix, DingTalk, Feishu/Lark, WeCom, Weixin, BlueBubbles (iMessage), QQ, Yuanbao, or your browser. The gateway is a single background process that connects to all your configured platforms, handles sessions, runs cron jobs, and delivers voice messages. +Chat with Hermes from Telegram, Discord, Slack, WhatsApp, Signal, SMS, Email, Home Assistant, Mattermost, Matrix, DingTalk, Feishu/Lark, WeCom, Weixin, BlueBubbles (iMessage), QQ, Yuanbao, Microsoft Teams, or your browser. The gateway is a single background process that connects to all your configured platforms, handles sessions, runs cron jobs, and delivers voice messages. For the full voice feature set — including CLI microphone mode, spoken replies in messaging, and Discord voice-channel conversations — see [Voice Mode](/docs/user-guide/features/voice-mode) and [Use Voice Mode with Hermes](/docs/guides/use-voice-mode-with-hermes). @@ -32,6 +32,7 @@ For the full voice feature set — including CLI microphone mode, spoken replies | BlueBubbles | — | ✅ | ✅ | — | ✅ | ✅ | — | | QQ | ✅ | ✅ | ✅ | — | — | ✅ | — | | Yuanbao | ✅ | ✅ | ✅ | — | — | ✅ | ✅ | +| Microsoft Teams | — | ✅ | — | — | — | ✅ | — | **Voice** = TTS audio replies and/or voice message transcription. **Images** = send/receive images. **Files** = send/receive file attachments. **Threads** = threaded conversations. **Reactions** = emoji reactions on messages. **Typing** = typing indicator while processing. **Streaming** = progressive message updates via editing. @@ -59,6 +60,7 @@ flowchart TB bb[BlueBubbles] qq[QQ] yb[Yuanbao] + ms[Microsoft Teams] api["API Server
(OpenAI-compatible)"] wh[Webhooks] end @@ -86,6 +88,7 @@ flowchart TB bb --> store qq --> store yb --> store + ms --> store api --> store wh --> store store --> agent @@ -189,6 +192,7 @@ DINGTALK_ALLOWED_USERS=user-id-1 FEISHU_ALLOWED_USERS=ou_xxxxxxxx,ou_yyyyyyyy WECOM_ALLOWED_USERS=user-id-1,user-id-2 WECOM_CALLBACK_ALLOWED_USERS=user-id-1,user-id-2 +TEAMS_ALLOWED_USERS=aad-object-id-1,aad-object-id-2 # Or allow GATEWAY_ALLOWED_USERS=123456789,987654321 @@ -393,6 +397,7 @@ Each platform has its own toolset: | BlueBubbles | `hermes-bluebubbles` | Full tools including terminal | | QQBot | `hermes-qqbot` | Full tools including terminal | | Yuanbao | `hermes-yuanbao` | Full tools including terminal | +| Microsoft Teams | `hermes-teams` | Full tools including terminal | | API Server | `hermes` (default) | Full tools including terminal | | Webhooks | `hermes-webhook` | Full tools including terminal | @@ -416,5 +421,6 @@ Each platform has its own toolset: - [BlueBubbles Setup (iMessage)](bluebubbles.md) - [QQBot Setup](qqbot.md) - [Yuanbao Setup](yuanbao.md) +- [Microsoft Teams Setup](teams.md) - [Open WebUI + API Server](open-webui.md) - [Webhooks](webhooks.md) \ No newline at end of file From 69aeba0df782b3383480ffbf0733d725a714c0a1 Mon Sep 17 00:00:00 2001 From: Aamir Jawaid Date: Fri, 1 May 2026 04:57:59 +0000 Subject: [PATCH 18/21] feat(teams): implement threading via app.reply() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wire reply_to into send() using App.reply(conv_id, msg_id, content) which constructs the threaded conversation ID internally. Threads supported in channels and group chats. Update comparison table: Threads ✅ Co-Authored-By: Claude Sonnet 4.6 --- plugins/platforms/teams/adapter.py | 5 ++++- website/docs/user-guide/messaging/index.md | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/plugins/platforms/teams/adapter.py b/plugins/platforms/teams/adapter.py index d0a2b7adbc..9bc4ac3499 100644 --- a/plugins/platforms/teams/adapter.py +++ b/plugins/platforms/teams/adapter.py @@ -509,7 +509,10 @@ class TeamsAdapter(BasePlatformAdapter): for chunk in chunks: try: - result = await self._app.send(chat_id, chunk) + if reply_to: + result = await self._app.reply(chat_id, reply_to, chunk) + else: + result = await self._app.send(chat_id, chunk) last_message_id = getattr(result, "id", None) except Exception as e: return SendResult(success=False, error=str(e), retryable=True) diff --git a/website/docs/user-guide/messaging/index.md b/website/docs/user-guide/messaging/index.md index 07342e8cf8..c68bdae91c 100644 --- a/website/docs/user-guide/messaging/index.md +++ b/website/docs/user-guide/messaging/index.md @@ -32,7 +32,7 @@ For the full voice feature set — including CLI microphone mode, spoken replies | BlueBubbles | — | ✅ | ✅ | — | ✅ | ✅ | — | | QQ | ✅ | ✅ | ✅ | — | — | ✅ | — | | Yuanbao | ✅ | ✅ | ✅ | — | — | ✅ | ✅ | -| Microsoft Teams | — | ✅ | — | — | — | ✅ | — | +| Microsoft Teams | — | ✅ | — | ✅ | — | ✅ | — | **Voice** = TTS audio replies and/or voice message transcription. **Images** = send/receive images. **Files** = send/receive file attachments. **Threads** = threaded conversations. **Reactions** = emoji reactions on messages. **Typing** = typing indicator while processing. **Streaming** = progressive message updates via editing. From 3f023450ddab8af48814fd715d3f0ee4318026ee Mon Sep 17 00:00:00 2001 From: Aamir Jawaid Date: Fri, 1 May 2026 05:21:28 +0000 Subject: [PATCH 19/21] fix(teams): fall back to flat send when threading returns 400 Group chats return 400 for threaded sends. Catch the error and fall back to a flat send so messages always get delivered. Co-Authored-By: Claude Sonnet 4.6 --- plugins/platforms/teams/adapter.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/plugins/platforms/teams/adapter.py b/plugins/platforms/teams/adapter.py index 9bc4ac3499..2729de8f41 100644 --- a/plugins/platforms/teams/adapter.py +++ b/plugins/platforms/teams/adapter.py @@ -509,8 +509,11 @@ class TeamsAdapter(BasePlatformAdapter): for chunk in chunks: try: - if reply_to: - result = await self._app.reply(chat_id, reply_to, chunk) + if reply_to and reply_to.isdigit() and reply_to != "0": + try: + result = await self._app.reply(chat_id, reply_to, chunk) + except Exception: + result = await self._app.send(chat_id, chunk) else: result = await self._app.send(chat_id, chunk) last_message_id = getattr(result, "id", None) From 2333b7a7ec682c999d9bfa9dd96ce8c293c86330 Mon Sep 17 00:00:00 2001 From: Aamir Jawaid Date: Fri, 1 May 2026 06:26:57 +0000 Subject: [PATCH 20/21] fix(tests): patch TypingActivityInput after mock on Python <3.12 The SDK requires Python >=3.12 so CI (3.11) falls to the except ImportError branch, leaving TypingActivityInput=None. After loading the adapter module, explicitly restore it from the mock so test_send_typing doesn't silently no-op. Co-Authored-By: Claude Sonnet 4.6 --- tests/gateway/test_teams.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/gateway/test_teams.py b/tests/gateway/test_teams.py index 28442788a5..0e1e05bd1b 100644 --- a/tests/gateway/test_teams.py +++ b/tests/gateway/test_teams.py @@ -169,6 +169,13 @@ _teams_mod = load_plugin_adapter("teams") _teams_mod.TEAMS_SDK_AVAILABLE = True _teams_mod.AIOHTTP_AVAILABLE = True +# Ensure SDK symbols that were None (import failed on Python <3.12) are +# replaced with the mocked versions so runtime calls don't silently no-op. +import sys as _sys +_mt = _sys.modules.get("microsoft_teams.api.activities.typing") +if _mt and _teams_mod.TypingActivityInput is None: + _teams_mod.TypingActivityInput = _mt.TypingActivityInput + TeamsAdapter = _teams_mod.TeamsAdapter check_requirements = _teams_mod.check_requirements check_teams_requirements = _teams_mod.check_teams_requirements From 601e5f1d57cfd4ceefee50a6df05a860a1a602e8 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 4 May 2026 20:53:57 -0700 Subject: [PATCH 21/21] fix(teams): log reply() fallback for diagnostics The previous bare except swallowed every exception from app.reply() silently. Log at debug so real failures (auth, chat gone) leave a trace while keeping the group-chat 400 fallback working. Also fix the Teams entry's indentation in the messaging flowchart. --- plugins/platforms/teams/adapter.py | 9 ++++++++- website/docs/user-guide/messaging/index.md | 6 +++--- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/plugins/platforms/teams/adapter.py b/plugins/platforms/teams/adapter.py index 2729de8f41..cdec7e3f1e 100644 --- a/plugins/platforms/teams/adapter.py +++ b/plugins/platforms/teams/adapter.py @@ -512,7 +512,14 @@ class TeamsAdapter(BasePlatformAdapter): if reply_to and reply_to.isdigit() and reply_to != "0": try: result = await self._app.reply(chat_id, reply_to, chunk) - except Exception: + except Exception as reply_err: + # Group chats 400 on threaded sends; the Teams SDK + # doesn't expose typed HTTP errors, so fall back on + # any exception and log for diagnostics. + logger.debug( + "Teams reply() failed, falling back to flat send: %s", + reply_err, + ) result = await self._app.send(chat_id, chunk) else: result = await self._app.send(chat_id, chunk) diff --git a/website/docs/user-guide/messaging/index.md b/website/docs/user-guide/messaging/index.md index c68bdae91c..25e8e4598f 100644 --- a/website/docs/user-guide/messaging/index.md +++ b/website/docs/user-guide/messaging/index.md @@ -60,9 +60,9 @@ flowchart TB bb[BlueBubbles] qq[QQ] yb[Yuanbao] - ms[Microsoft Teams] - api["API Server
(OpenAI-compatible)"] - wh[Webhooks] + ms[Microsoft Teams] + api["API Server
(OpenAI-compatible)"] + wh[Webhooks] end store["Session store
per chat"]