From ed69aa9e6ede9be46e778aaf8cd32fecdcfa5629 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Mon, 21 Sep 2026 15:28:45 -0700 Subject: [PATCH] refactor(doctor,gateway): share the migrate preflight's duplicate-credential check with doctor and status Under multiplex-only the duplicated platform token is exactly what the migration preflight refuses to fold on, so doctor and `gateway status` reuse that check (`gateway_migrate.duplicate_credential_findings`) instead of a second scanner in profile_channels. All three surfaces print one shared line naming the profiles, the env key NAME (TELEGRAM_BOT_TOKEN, never a value or fingerprint), why one token can serve only one gateway, and the remedy (own token / remove the key from the non-owner / profile_routes) ending in `hermes gateway migrate --multiplex`. Doctor folds the finding into its existing Profiles section. Tests trimmed to two invariants: identical wording across doctor, status and the preflight with the secret absent from output; distinct tokens and a shared non-singleton key (model API key) produce no finding. --- hermes_cli/doctor.py | 3 +- hermes_cli/doctor_state.py | 30 +---- hermes_cli/gateway.py | 29 ++--- hermes_cli/gateway_migrate.py | 60 ++++++--- hermes_cli/profile_channels.py | 82 ------------ .../test_profile_credential_collisions.py | 119 +++++++----------- 6 files changed, 109 insertions(+), 214 deletions(-) diff --git a/hermes_cli/doctor.py b/hermes_cli/doctor.py index 1fcea9dac0..107b563b63 100644 --- a/hermes_cli/doctor.py +++ b/hermes_cli/doctor.py @@ -49,7 +49,6 @@ from hermes_cli.doctor_tools import ( ) from hermes_cli.doctor_state import ( _check_checkpoint_store, - _check_cross_profile_gateway_credentials, _check_directory_structure, _check_memory_provider, _check_profiles, @@ -120,7 +119,7 @@ DOCTOR_CHECKS = ( ('External Tools', _check_git_and_rg), (None, _check_terminal_backend), (None, _check_node_and_browser), (None, _check_npm_audit), ('API Connectivity', _check_api_connectivity), ('Tool Availability', _check_tool_availability), ('Skills Hub', _check_skills_hub), - ('Memory Provider', _check_memory_provider), (None, _check_profiles), (None, _check_cross_profile_gateway_credentials), + ('Memory Provider', _check_memory_provider), (None, _check_profiles), ) diff --git a/hermes_cli/doctor_state.py b/hermes_cli/doctor_state.py index 349a290ef3..60972cdf4e 100644 --- a/hermes_cli/doctor_state.py +++ b/hermes_cli/doctor_state.py @@ -572,27 +572,9 @@ def _check_profiles(should_fix: bool, f: Finding) -> None: _m = _re.search(r"hermes -p (\S+)", wrapper.read_text(encoding="utf-8")) if _m and not profile_exists(_m.group(1)): check_warn(f"Orphan alias: {wrapper.name} → profile '{_m.group(1)}' no longer exists") - - -@doctor_check("") # Diagnostics must still finish if one profile's files are unreadable. -def _check_cross_profile_gateway_credentials(should_fix: bool, f: Finding) -> None: - """Report bot credentials that would make local profile gateways fight each other.""" - from hermes_cli.profile_channels import scan_local_profile_credential_collisions - - report = scan_local_profile_credential_collisions() - if not report.collisions and not report.unreadable_paths: - return - _section("Gateway Profile Credentials") - if report.collisions: - check_warn("Duplicate platform credentials across local profile homes", f"({report.format_for_display()})") - f.manual_issues.append( - "Duplicate gateway platform credentials across local profiles — give each profile its own bot " - "credential or stop/remove the other profile gateway before starting it." - ) - if report.unreadable_paths: - paths = ", ".join(str(path) for path in report.unreadable_paths) - check_warn("Could not inspect gateway credentials in local profile homes", f"({paths})") - f.manual_issues.append( - "Could not inspect one or more local profile homes for duplicate gateway credentials; " - "check their path permissions and configuration files." - ) + # Same helper as the multiplex migration preflight, so doctor names the duplicates that make + # `hermes gateway migrate --multiplex` refuse (and made pre-multiplex standalone gateways race). + from hermes_cli.gateway_migrate import duplicate_credential_findings + for line in duplicate_credential_findings(): + check_warn("Duplicate platform credential across profiles", f"({line})") + f.manual_issues.append(line) diff --git a/hermes_cli/gateway.py b/hermes_cli/gateway.py index 26e4db2d44..2c4cd891e3 100644 --- a/hermes_cli/gateway.py +++ b/hermes_cli/gateway.py @@ -1600,23 +1600,16 @@ def _print_other_profiles_gateway_status() -> None: pass -def _print_cross_profile_credential_warnings() -> None: - """Surface bot-token collisions even when profile gateways run standalone.""" - try: - from hermes_cli.profile_channels import scan_local_profile_credential_collisions - - report = scan_local_profile_credential_collisions() - except Exception: - return - if not report.collisions and not report.unreadable_paths: - return - print() - if report.collisions: - print("⚠ Duplicate platform credentials across local profile homes") - print(f" {report.format_for_display()}") - print(" Give each profile its own bot credential, or stop/remove the other profile gateway.") - if report.unreadable_paths: - print("⚠ Could not inspect gateway credentials in: " + ", ".join(map(str, report.unreadable_paths))) +def _print_duplicate_credential_warnings() -> None: + """The migrate preflight's duplicate-credential findings, so ``gateway status`` explains a parked + or racing bot (and why the fleet will not fold) with the same words as ``migrate --dry-run``.""" + with contextlib.suppress(Exception): + from hermes_cli.gateway_migrate import duplicate_credential_findings + lines = duplicate_credential_findings() + if lines: + print() + for line in lines: + print(f"⚠ {line}") def _gateway_list() -> None: @@ -5040,7 +5033,7 @@ def _cmd_status(args): print(" hermes gateway run # Run in foreground") _print_lines(*_STATUS_STOPPED_HINTS[_status_host_kind()]) - _print_cross_profile_credential_warnings() + _print_duplicate_credential_warnings() _print_other_profiles_gateway_status() diff --git a/hermes_cli/gateway_migrate.py b/hermes_cli/gateway_migrate.py index 1b8358f774..dc27f3fd7a 100644 --- a/hermes_cli/gateway_migrate.py +++ b/hermes_cli/gateway_migrate.py @@ -396,24 +396,54 @@ def _credential_claims(config) -> dict[tuple, str]: return claims +def _credential_key_names(platform_value: str) -> str: + """Env key NAMES (never values) that make ``platform_value`` connect as a bot, e.g. + ``TELEGRAM_BOT_TOKEN``; the platform id when no key is registered (config.yaml-only token).""" + from hermes_cli.profile_channels import credential_env_keys + names = sorted(key for key, pid in credential_env_keys().items() if pid == platform_value) + return "/".join(names) or f"the {platform_value} token" + + +def duplicate_credential_lines(configs: list[tuple[str, object]]) -> list[str]: + """One finding per platform credential two profiles both hold, with the remedy. The SINGLE + source for the migrate preflight, ``hermes doctor`` and ``hermes gateway status``, so all three + name the same duplicates the same way: profile names + key names only, never a value or hash.""" + owners: dict[tuple, str] = {} + lines: list[str] = [] + for name, cfg in configs: # default first: it wins the claim, like at multiplexer startup + for claim, platform_value in _credential_claims(cfg).items(): + owner = owners.setdefault(claim, name) + if owner == name: + continue + key = _credential_key_names(platform_value) + lines.append( + f"Profiles '{owner}' and '{name}' both hold the same {platform_value} credential ({key}): " + f"one platform token can serve only one gateway, so the bot answers from whichever profile " + f"claims it first and the other's adapter is parked. Give '{name}' its own bot token, or remove " + f"{key} from the profile that should not own it (or keep it in {owner} and route {name}'s " + f"chats with profile_routes — gateway.profile_routes in {owner}'s config.yaml), then run " + f"{MIGRATE_COMMAND}." + ) + return lines + + +def duplicate_credential_findings() -> list[str]: + """The preflight's duplicate-credential check read straight from the local profile homes, for + diagnostics that have no migration plan (doctor, gateway status). A profile whose gateway config + does not load is skipped here — ``build_migration_plan`` reports that one as its own blocker.""" + configs: list[tuple[str, object]] = [] + with _multiplex_read_mode(): + for name, home in _profile_homes(): + with contextlib.suppress(Exception): + configs.append((name, _profile_gateway_config(home))) + return duplicate_credential_lines(configs) + + def _check_duplicate_credentials(plan: MigrationPlan, configs: dict[str, object]) -> None: """BLOCKER: the same bot credential configured on two profiles — the multiplexer would park the duplicate adapter, so one profile's bot would go silent after migration.""" - owners: dict[tuple, str] = {} - for profile in plan.profiles: # default first: it wins the claim, like at multiplexer startup - cfg = configs.get(profile.name) - if cfg is None: - continue - for claim, platform_value in _credential_claims(cfg).items(): - owner = owners.setdefault(claim, profile.name) - if owner == profile.name: - continue - plan.blockers.append( - f"Profiles '{owner}' and '{profile.name}' both configure {platform_value} with the same " - f"credential: the bot can only belong to one profile; remove the token from " - f"'{profile.name}' or keep it in {owner} and route {profile.name}'s chats with " - f"profile_routes (gateway.profile_routes in {owner}'s config.yaml)." - ) + plan.blockers.extend(duplicate_credential_lines( + [(p.name, configs[p.name]) for p in plan.profiles if p.name in configs])) def platform_serves_profile_prefix(platform_value: str) -> bool: diff --git a/hermes_cli/profile_channels.py b/hermes_cli/profile_channels.py index 41d8d35be2..70c8f12c47 100644 --- a/hermes_cli/profile_channels.py +++ b/hermes_cli/profile_channels.py @@ -22,7 +22,6 @@ from __future__ import annotations import contextlib import logging import re -from dataclasses import dataclass from functools import partial from pathlib import Path from typing import Dict, Iterable, List, Optional, Set, Tuple @@ -483,87 +482,6 @@ def shared_credential_warning(profile: str, platforms: List[str], source: str = ) -@dataclass(frozen=True) -class ProfileCredentialCollision: - """One platform credential claimed by more than one local profile home. - - ``paths`` intentionally names homes, not the credential or its fingerprint: doctor and - gateway status must make the operator's next action clear without leaking a secret. - """ - - platform: str - paths: tuple[Path, ...] - - -@dataclass(frozen=True) -class LocalProfileCredentialCollisionReport: - """Best-effort credential scan used by non-mutating diagnostic commands.""" - - collisions: tuple[ProfileCredentialCollision, ...] = () - unreadable_paths: tuple[Path, ...] = () - - def format_for_display(self) -> str: - """Render only platform names and paths; never expose credentials or fingerprints.""" - return "; ".join( - f"{collision.platform}: {', '.join(str(path) for path in collision.paths)}" - for collision in self.collisions - ) - - -def _local_profile_homes() -> tuple[tuple[str, Path], ...]: - """Distinct default/named homes rooted at the local Hermes installation.""" - from hermes_cli.profiles import profiles_to_serve - - homes: list[tuple[str, Path]] = [] - seen: set[Path] = set() - for name, home in profiles_to_serve(multiplex=True): - try: - key = home.resolve(strict=False) - except OSError: - key = home - if key not in seen: - seen.add(key) - homes.append((name, home)) - return tuple(homes) - - -def _profile_credential_claims(home: Path) -> set[tuple[str, str]]: - """Read claims through the migration preflight's gateway fingerprint implementation.""" - from hermes_cli import gateway_migrate - - with gateway_migrate._multiplex_read_mode(): - return set(gateway_migrate._credential_claims(gateway_migrate._profile_gateway_config(home))) - - -def scan_local_profile_credential_collisions() -> LocalProfileCredentialCollisionReport: - """Find duplicate enabled platform credentials across local profile homes. - - The gateway migration preflight is the canonical source for both enabled-platform - resolution and credential fingerprints. This scan deliberately uses the same path so a - diagnostic warning agrees with the gateway's collision behaviour. A malformed or unreadable - profile is reported separately while healthy homes continue to be scanned. - """ - owners: dict[tuple[str, str], list[Path]] = {} - unreadable: list[Path] = [] - for _name, home in _local_profile_homes(): - try: - claims = _profile_credential_claims(home) - except (OSError, ValueError, RuntimeError): - unreadable.append(home) - continue - for claim in claims: - owners.setdefault(claim, []).append(home) - collisions = tuple( - ProfileCredentialCollision(platform=platform, paths=tuple(paths)) - for (platform, _fingerprint), paths in sorted(owners.items()) - if len(paths) > 1 - ) - return LocalProfileCredentialCollisionReport( - collisions=collisions, - unreadable_paths=tuple(unreadable), - ) - - def format_stripped_notice(profile: str, platforms: List[str], clone_flag: str = "--clone") -> List[str]: """Lines printed after a channel-less clone so the user knows what was left behind and how to configure the new profile's own bots.""" diff --git a/tests/hermes_cli/test_profile_credential_collisions.py b/tests/hermes_cli/test_profile_credential_collisions.py index 776ca62a8b..46a1c0d42c 100644 --- a/tests/hermes_cli/test_profile_credential_collisions.py +++ b/tests/hermes_cli/test_profile_credential_collisions.py @@ -1,4 +1,7 @@ -"""Regression coverage for cross-profile gateway credential diagnostics (#118388).""" +"""Regression for #118388: a platform singleton secret (``TELEGRAM_BOT_TOKEN``) duplicated across +local profile homes was invisible to ``hermes doctor`` / ``hermes gateway status``; the only signal +was the losing standalone gateway's log. Doctor, status and the migrate preflight now share one +duplicate-credential helper, so all three name the same profiles + key names (never the value).""" from __future__ import annotations @@ -6,89 +9,59 @@ import io from contextlib import redirect_stdout from pathlib import Path +import pytest + import hermes_constants -from hermes_cli import doctor_state, profile_channels +from hermes_cli import doctor_state, gateway, gateway_migrate as gm + +SECRET = "123456:shared-secret-value" -def _profile_home(root: Path, name: str, token: str) -> Path: - home = root if name == "default" else root / "profiles" / name - home.mkdir(parents=True, exist_ok=True) - (home / ".env").write_text(f"TELEGRAM_BOT_TOKEN={token}\n", encoding="utf-8") - return home - - -def test_scan_reports_duplicate_platform_credential_with_paths_not_secret(monkeypatch, tmp_path): +@pytest.fixture +def homes(tmp_path, monkeypatch): root = tmp_path / ".hermes" - default = _profile_home(root, "default", "123:shared-secret") - worker = _profile_home(root, "worker", "123:shared-secret") + (root / "profiles" / "worker").mkdir(parents=True) + monkeypatch.setattr(Path, "home", lambda: tmp_path) monkeypatch.setenv("HERMES_HOME", str(root)) + for name in ("TELEGRAM_BOT_TOKEN", "DISCORD_BOT_TOKEN", "OPENAI_API_KEY"): + monkeypatch.delenv(name, raising=False) monkeypatch.setattr(hermes_constants, "_default_hermes_root_memo", None) - - report = profile_channels.scan_local_profile_credential_collisions() - - assert [(collision.platform, collision.paths) for collision in report.collisions] == [ - ("telegram", (default, worker)), - ] - assert "shared-secret" not in report.format_for_display() - assert str(default) in report.format_for_display() - assert str(worker) in report.format_for_display() + monkeypatch.setattr(gm, "_live_gateway_pid", lambda home: None) + monkeypatch.setattr(gm, "_installed_services", lambda home: []) + return root, root / "profiles" / "worker" -def test_scan_ignores_distinct_platform_credentials(monkeypatch, tmp_path): - root = tmp_path / ".hermes" - _profile_home(root, "default", "123:default-secret") - _profile_home(root, "worker", "123:worker-secret") - monkeypatch.setenv("HERMES_HOME", str(root)) - monkeypatch.setattr(hermes_constants, "_default_hermes_root_memo", None) - - report = profile_channels.scan_local_profile_credential_collisions() - - assert report.collisions == () - assert report.unreadable_paths == () +def _surfaces() -> str: + out = io.StringIO() + with redirect_stdout(out): + doctor_state._check_profiles(False) + gateway._print_duplicate_credential_warnings() + return out.getvalue() -def test_scan_reports_unreadable_profile_without_hiding_other_collisions(monkeypatch, tmp_path): - default = tmp_path / ".hermes" - worker = default / "profiles" / "worker" - broken = default / "profiles" / "broken" - monkeypatch.setattr( - profile_channels, - "_local_profile_homes", - lambda: (("default", default), ("worker", worker), ("broken", broken)), - ) - monkeypatch.setattr( - profile_channels, - "_profile_credential_claims", - lambda home: ( - {("telegram", "fingerprint")} if home != broken else (_ for _ in ()).throw(OSError("denied")) - ), - ) +def test_duplicate_singleton_secret_named_identically_by_doctor_status_and_preflight(homes): + default, worker = homes + (default / ".env").write_text(f"TELEGRAM_BOT_TOKEN={SECRET}\nOPENAI_API_KEY=sk-shared\n", encoding="utf-8") + (worker / ".env").write_text(f"TELEGRAM_BOT_TOKEN={SECRET}\nOPENAI_API_KEY=sk-shared\n", encoding="utf-8") - report = profile_channels.scan_local_profile_credential_collisions() - - assert report.collisions[0].platform == "telegram" - assert report.unreadable_paths == (broken,) + findings = gm.duplicate_credential_findings() + assert len(findings) == 1 + line = findings[0] + assert "'default'" in line and "'worker'" in line and "TELEGRAM_BOT_TOKEN" in line + assert "only one gateway" in line and gm.MIGRATE_COMMAND in line + # Same helper, same words: the preflight blocker IS the doctor/status finding. + assert gm.build_migration_plan().blockers == findings + out = _surfaces() + assert out.count(line) == 2 # once from doctor, once from gateway status + # Absence: the value (or a hash a user could mistake for it) never reaches any surface. + assert SECRET not in out and "shared-secret" not in out and "sk-shared" not in out -def test_doctor_and_gateway_status_surface_collision_without_secret(monkeypatch, tmp_path): - default = tmp_path / ".hermes" - worker = default / "profiles" / "worker" - report = profile_channels.LocalProfileCredentialCollisionReport( - collisions=(profile_channels.ProfileCredentialCollision("telegram", (default, worker)),), - ) - monkeypatch.setattr(profile_channels, "scan_local_profile_credential_collisions", lambda: report) +def test_distinct_tokens_and_shared_non_singleton_keys_are_not_findings(homes): + default, worker = homes + (default / ".env").write_text(f"TELEGRAM_BOT_TOKEN={SECRET}\nOPENAI_API_KEY=sk-shared\n", encoding="utf-8") + (worker / ".env").write_text("TELEGRAM_BOT_TOKEN=999999:other-token\nOPENAI_API_KEY=sk-shared\n", encoding="utf-8") - doctor_output = io.StringIO() - with redirect_stdout(doctor_output): - findings = doctor_state._check_cross_profile_gateway_credentials(False) - assert "Duplicate platform credentials" in doctor_output.getvalue() - assert str(worker) in doctor_output.getvalue() - assert findings.manual_issues - - from hermes_cli import gateway - - status_output = io.StringIO() - with redirect_stdout(status_output): - gateway._print_cross_profile_credential_warnings() - assert "Duplicate platform credentials" in status_output.getvalue() - assert "shared-secret" not in status_output.getvalue() + assert gm.duplicate_credential_findings() == [] + assert not gm.build_migration_plan().blocked + assert "both hold" not in _surfaces()