From ebbd5821fd93464e9a27946fdefddc39c34b1ec1 Mon Sep 17 00:00:00 2001 From: Donald Date: Sat, 3 Oct 2026 02:58:45 +0000 Subject: [PATCH] Harden Hub startup and display-name handling - Display names starting with '-' were parsed by tmux rename-window as flags. The alias was persisted before tmux rejected it, so every later start/refresh/open failed in refresh_labels until aliases.json was edited by hand. Guard rename-window with '--', apply the name before persisting it, and roll back the in-memory table on failure. - Start the Hub tmux server with -f /dev/null by default. Reading the user's ~/.tmux.conf let theme status-formats replace the Sessions bar and root-table bindings leak into Herdr; HERDR_HUB_TMUX_CONFIG still opts into a custom file. - An unreadable aliases.json blocked start/refresh/open/choose. Degrade to default names with a warning (stderr, or the popup message) and leave the file untouched for `doctor` to report. - `doctor` crashed with a traceback when a *_BIN override pointed at a missing executable; report it as a failure instead. - Refuse to run inside another tmux server before discovery and server creation, so no unattached Hub is left behind. Co-Authored-By: Claude Fable 5.1 --- herdr_hub/cli.py | 109 ++++++++++++++++++++++++++------- tests/test_cli_integration.py | 11 ++++ tests/test_discovery.py | 75 +++++++++++++++++++++++ tests/test_tmux_integration.py | 10 +++ 4 files changed, 184 insertions(+), 21 deletions(-) diff --git a/herdr_hub/cli.py b/herdr_hub/cli.py index 2512aa4..e3b3e53 100644 --- a/herdr_hub/cli.py +++ b/herdr_hub/cli.py @@ -47,6 +47,9 @@ from .selector import ( DEFAULT_SOCKET_NAME = "herdr-hub" DEFAULT_TMUX_SESSION = "hub" +# The Hub server is configured entirely by configure(); reading the user's +# ~/.tmux.conf would let theme status-formats and root-table bindings leak in. +DEFAULT_TMUX_CONFIG = "/dev/null" DEFAULT_SESSION_NAME = "default" BOOTSTRAP_WINDOW_NAME = "__herdr_hub_bootstrap__" WINDOW_KEY_OPTION = "@herdr_hub_key" @@ -414,7 +417,7 @@ def discover_remote_processes( raw = cmdline_path.read_bytes() except (FileNotFoundError, ProcessLookupError): continue - except (PermissionError, OSError): + except OSError: unreadable += 1 continue endpoint = parse_remote_argv(decode_cmdline(raw)) @@ -749,7 +752,10 @@ class TmuxHub: truncate_label(alias, 32) if alias else window.endpoint.short_label ) if window.name != display_label: - self.run(["rename-window", "-t", window.window_id, display_label]) + # A display name may start with '-', which tmux would otherwise + # parse as a flag. set-option takes the value positionally and + # does not accept '--', so only rename-window needs the guard. + self.run(["rename-window", "-t", window.window_id, "--", display_label]) self.run( [ "set-option", @@ -763,13 +769,22 @@ class TmuxHub: @serialized_hub_mutation def apply_alias(self, key: str, label: str) -> ManagedWindow | None: + previous = self.aliases.get(key) if label: self.aliases[key] = label else: self.aliases.pop(key, None) window = self.window_for_key(key) if window is not None: - self._apply_window_label(window) + try: + self._apply_window_label(window) + except HubError: + # Keep the in-memory table consistent with what tmux shows. + if previous is None: + self.aliases.pop(key, None) + else: + self.aliases[key] = previous + raise return self.window_for_key(key) def dismissed_keys(self) -> set[str]: @@ -1065,12 +1080,9 @@ class TmuxHub: time.sleep(min(0.05, remaining)) def attach(self) -> None: - if inside_any_tmux(): - if inside_socket(self.socket_name): - return - raise HubError( - "already inside another tmux server; detach from it before attaching to herdr-hub" - ) + if inside_socket(self.socket_name): + return + ensure_not_nested(self.socket_name) result = self.run( ["attach-session", "-t", self.session_name], check=False, @@ -1117,20 +1129,50 @@ def invocation_script_path() -> str: return str(invoked) +def configured_socket_name() -> str: + return os.environ.get("HERDR_HUB_SOCKET", DEFAULT_SOCKET_NAME) + + +def configured_tmux_config() -> str: + override = os.environ.get("HERDR_HUB_TMUX_CONFIG") + return os.path.expanduser(override) if override else DEFAULT_TMUX_CONFIG + + +def load_alias_table() -> tuple[dict[str, str], str | None]: + """Load display names, degrading to defaults when the file is unreadable. + + A corrupt optional file must not block start/refresh/open; `doctor` + reports the problem and the warning is surfaced to the caller. + """ + registry = make_alias_registry() + try: + return registry.load(), None + except AliasRegistryError as error: + return {}, f"display names unavailable, using defaults: {error}" + + def make_hub( arguments: argparse.Namespace, *, load_aliases: bool = True, + warnings: list[str] | None = None, ) -> TmuxHub: tmux_bin = resolve_binary("tmux", os.environ.get("HERDR_HUB_TMUX_BIN")) ssh_bin = os.environ.get("HERDR_HUB_SSH_BIN") or shutil.which("ssh") or "ssh" - aliases = make_alias_registry().load() if load_aliases else {} + aliases: dict[str, str] = {} + if load_aliases: + aliases, alias_warning = load_alias_table() + if alias_warning is not None: + if warnings is not None: + warnings.append(alias_warning) + else: + print(f"herdr-hub: warning: {alias_warning}", file=sys.stderr) return TmuxHub( tmux_bin=tmux_bin, ssh_bin=ssh_bin, - socket_name=os.environ.get("HERDR_HUB_SOCKET", DEFAULT_SOCKET_NAME), + socket_name=configured_socket_name(), session_name=os.environ.get("HERDR_HUB_SESSION", DEFAULT_TMUX_SESSION), - config_file=os.environ.get("HERDR_HUB_TMUX_CONFIG"), + config_file=configured_tmux_config(), script_path=invocation_script_path(), aliases=aliases, ) @@ -1199,7 +1241,17 @@ def catalog_windows(windows: Iterable[ManagedWindow]) -> list[CatalogWindow]: ] +def ensure_not_nested(socket_name: str) -> None: + if inside_any_tmux() and not inside_socket(socket_name): + raise HubError( + "already inside another tmux server; detach from it before attaching to herdr-hub" + ) + + def command_start(arguments: argparse.Namespace) -> int: + # Fail before discovery and server creation: a Hub nobody can attach to + # would otherwise be left running in the background. + ensure_not_nested(configured_socket_name()) herdr_bin = resolve_herdr() report = discover_endpoints(herdr_bin, configured_proc_root()) if warning := remember_remote_endpoints(report.endpoints): @@ -1280,7 +1332,8 @@ def command_open(arguments: argparse.Namespace) -> int: def command_choose(arguments: argparse.Namespace) -> int: - hub = make_hub(arguments) + startup_warnings: list[str] = [] + hub = make_hub(arguments, warnings=startup_warnings) if not hub.exists(): raise HubError("herdr-hub is not running") windows = hub.managed_windows() @@ -1298,9 +1351,17 @@ def command_choose(arguments: argparse.Namespace) -> int: return catalog_windows(hub.managed_windows()) def rename_session(endpoint_key: str, alias: str) -> None: - alias_registry.set(endpoint_key, alias) + # Apply to tmux first: a name tmux rejects must never be persisted, + # or every later start/refresh would fail while re-applying it. + previous = hub.aliases.get(endpoint_key, "") if hub.apply_alias(endpoint_key, alias) is None: + hub.apply_alias(endpoint_key, previous) raise HubError("the selected Session is no longer open") + try: + alias_registry.set(endpoint_key, alias) + except AliasRegistryError: + hub.apply_alias(endpoint_key, previous) + raise def delete_session( target: str, @@ -1408,7 +1469,7 @@ def command_choose(arguments: argparse.Namespace) -> int: delete_session_handler=delete_session, stop_session_handler=stop_session, ) - startup_warnings = [*local_report.warnings] + startup_warnings.extend(local_report.warnings) if registry_warning: startup_warnings.append(registry_warning) if startup_warnings: @@ -1556,12 +1617,18 @@ def command_doctor(arguments: argparse.Namespace) -> int: failures += 1 continue version_args = [path, "-V"] if binary in {"tmux", "ssh"} else [path, "--version"] - result = subprocess.run( - version_args, - capture_output=True, - text=True, - check=False, - ) + try: + result = subprocess.run( + version_args, + capture_output=True, + text=True, + check=False, + timeout=10, + ) + except (OSError, subprocess.SubprocessError) as error: + print(f"{binary}: cannot run {path} ({error})") + failures += 1 + continue version = (result.stdout or result.stderr).strip() print(f"{binary}: {version or path}") registry = make_registry() diff --git a/tests/test_cli_integration.py b/tests/test_cli_integration.py index 6df0cb6..68b3fd8 100644 --- a/tests/test_cli_integration.py +++ b/tests/test_cli_integration.py @@ -128,6 +128,17 @@ class CliIntegrationTests(unittest.TestCase): # Operational recovery must remain possible even if optional # display-name configuration is later corrupted. (root / "aliases.json").write_text("not json", encoding="utf-8") + degraded = subprocess.run( + [str(launcher), "refresh"], + cwd=repository_root, + env=environment, + capture_output=True, + text=True, + check=False, + ) + self.assertEqual(degraded.returncode, 0, degraded.stderr) + self.assertIn("display names unavailable", degraded.stderr) + self.assertEqual(hub.managed_windows()[0].name, "L default") stop = subprocess.run( [str(launcher), "stop"], cwd=repository_root, diff --git a/tests/test_discovery.py b/tests/test_discovery.py index 917fe36..fb82403 100644 --- a/tests/test_discovery.py +++ b/tests/test_discovery.py @@ -1,20 +1,29 @@ from __future__ import annotations +import argparse +import contextlib +import io import json import os from pathlib import Path import subprocess import tempfile import unittest +from unittest.mock import patch from herdr_hub.cli import ( DEFAULT_SESSION_NAME, + DEFAULT_TMUX_CONFIG, Endpoint, + HubError, + command_doctor, + command_start, decode_cmdline, dedupe_endpoints, deserialize_endpoint, discover_local_sessions, discover_remote_processes, + make_hub, parse_remote_argv, serialize_endpoint, ) @@ -199,5 +208,71 @@ class DiscoveryTests(unittest.TestCase): ) +class CliEnvironmentTests(unittest.TestCase): + def test_hub_ignores_user_tmux_conf_unless_overridden(self) -> None: + with tempfile.TemporaryDirectory() as directory: + environment = { + "HERDR_HUB_TMUX_BIN": "/bin/true", + "HERDR_HUB_ALIASES_FILE": str(Path(directory) / "aliases.json"), + } + with patch.dict(os.environ, environment): + os.environ.pop("HERDR_HUB_TMUX_CONFIG", None) + hub = make_hub(argparse.Namespace()) + self.assertEqual(hub.config_file, DEFAULT_TMUX_CONFIG) + self.assertEqual(hub.base_command()[-2:], ["-f", "/dev/null"]) + with patch.dict( + os.environ, {**environment, "HERDR_HUB_TMUX_CONFIG": "~/hub.conf"} + ): + hub = make_hub(argparse.Namespace()) + self.assertEqual(hub.config_file, os.path.expanduser("~/hub.conf")) + + def test_corrupt_alias_file_degrades_to_defaults_with_warning(self) -> None: + with tempfile.TemporaryDirectory() as directory: + aliases = Path(directory) / "aliases.json" + aliases.write_text("not json", encoding="utf-8") + environment = { + "HERDR_HUB_TMUX_BIN": "/bin/true", + "HERDR_HUB_ALIASES_FILE": str(aliases), + } + warnings: list[str] = [] + with patch.dict(os.environ, environment): + hub = make_hub(argparse.Namespace(), warnings=warnings) + self.assertEqual(hub.aliases, {}) + self.assertEqual(len(warnings), 1) + self.assertIn("aliases.json", warnings[0]) + + stderr = io.StringIO() + with patch.dict(os.environ, environment), contextlib.redirect_stderr(stderr): + make_hub(argparse.Namespace()) + self.assertIn("display names unavailable", stderr.getvalue()) + # The unreadable file is left untouched for the user to inspect. + self.assertEqual(aliases.read_text(encoding="utf-8"), "not json") + + def test_start_refuses_nested_tmux_before_discovery(self) -> None: + environment = {"TMUX": "/tmp/tmux-1/other,1,0", "HERDR_HUB_SOCKET": "herdr-hub"} + with patch.dict(os.environ, environment), patch( + "herdr_hub.cli.discover_endpoints", + side_effect=AssertionError("discovery must not run inside another tmux"), + ): + with self.assertRaisesRegex(HubError, "another tmux server"): + command_start(argparse.Namespace()) + + def test_doctor_reports_unusable_binary_override_instead_of_crashing(self) -> None: + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + environment = { + "HERDR_HUB_HERDR_BIN": str(root / "missing-herdr"), + "HERDR_HUB_REMOTES_FILE": str(root / "remotes.json"), + "HERDR_HUB_ALIASES_FILE": str(root / "aliases.json"), + "HERDR_HUB_PROC_ROOT": directory, + } + stdout = io.StringIO() + with patch.dict(os.environ, environment), contextlib.redirect_stdout(stdout): + status = command_doctor(argparse.Namespace()) + self.assertGreaterEqual(status, 1) + self.assertIn("herdr: cannot run", stdout.getvalue()) + self.assertIn("discovery failed", stdout.getvalue()) + + if __name__ == "__main__": unittest.main() diff --git a/tests/test_tmux_integration.py b/tests/test_tmux_integration.py index ea9bde6..5017b5e 100644 --- a/tests/test_tmux_integration.py +++ b/tests/test_tmux_integration.py @@ -310,6 +310,16 @@ class TmuxIntegrationTests(unittest.TestCase): renamed = self.hub.apply_alias(window.key, "临时名称") assert renamed is not None self.assertEqual(renamed.name, "临时名称") + # tmux parses a leading '-' as a flag unless the name is guarded; a + # stored name like this must also survive the label refresh that every + # later start/refresh performs. + dashed = self.hub.apply_alias(window.key, "-dash") + assert dashed is not None + self.assertEqual(dashed.name, "-dash") + self.hub.ensure_server() + refreshed = self.hub.window_for_key(window.key) + assert refreshed is not None + self.assertEqual(refreshed.name, "-dash") restored = self.hub.apply_alias(window.key, "") assert restored is not None self.assertEqual(restored.name, "R orangepi/work")