mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-21 15:41:15 +00:00
fix(profile-routing): remove dead forum cache, warn on missing profile
Three follow-ups to the initial routing PR after code review: 1. Remove dead forum-post hierarchy cache. The `_forum_post_cache`, `register_forum_post()`, and `resolve_forum_channel()` were never wired up — no caller in the codebase. Discord's adapter already sets `parent_chat_id` to the immediate parent (forum channel for a forum post), so the existing `self.chat_id == parent_chat_id` branch in `matches()` handles forum posts correctly without a cache. The hierarchical-resolution branch in `matches()` and the bounded-LRU infrastructure are removed. 2. Fix docstring specificity numbers (8 → 14, 4 → 6) and rewrite the "Hierarchical matching" section to describe the actual one-level parent_chat_id behavior. Removed unused `OrderedDict` and `Set` imports. 3. Warn loudly when a routed profile doesn't exist on disk. Previously, a typo in `profile_routes` (e.g. `crypto-tradr`) silently fell back to the global HERMES_HOME, causing the message to read the default profile's memory/credentials with no signal to the operator. Now emits a `logger.warning` with the profile name, source identifier, and the fallback reason. Bare-exception path also gets `exc_info`. Tests: - test_profile_routing.py: +2 tests verifying forum post matching via direct parent_chat_id (covers the case the removed cache was meant for). 31 total, all pass. - test_profile_resolution.py: NEW, 12 tests covering resolution order (source.profile > routing > active > default), missing-profile warning, exception handling, and routing consultation. All pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
parent
5e65f6d79f
commit
9166d727b8
4 changed files with 331 additions and 38 deletions
|
|
@ -4,15 +4,16 @@ Allows a single Hermes instance to route specific Discord guilds/channels/thread
|
|||
to different profiles — each with their own model, tools, memory, and persona.
|
||||
|
||||
Matching priority (most specific first):
|
||||
1. platform + chat_id + thread_id (exact thread) — specificity 8
|
||||
2. platform + chat_id (channel route) — specificity 4
|
||||
1. platform + chat_id + thread_id (exact thread) — specificity 14
|
||||
2. platform + chat_id (channel route) — specificity 6
|
||||
3. platform + guild_id (guild/server route) — specificity 2
|
||||
4. No match → default profile
|
||||
|
||||
Hierarchical matching:
|
||||
For Discord forum channels, checks the full parent chain:
|
||||
- Forum channel → Forum post → Comment
|
||||
- Matches if any level of the hierarchy matches a configured route
|
||||
Parent-chain matching:
|
||||
For Discord threads and forum posts, ``parent_chat_id`` carries the
|
||||
direct parent (the channel for a thread, the forum channel for a post).
|
||||
Routes keyed on a channel match both direct messages and messages in
|
||||
any thread/post whose parent is that channel.
|
||||
|
||||
Configuration (config.yaml):
|
||||
|
||||
|
|
@ -38,34 +39,14 @@ Configuration (config.yaml):
|
|||
|
||||
from __future__ import annotations
|
||||
|
||||
from collections import OrderedDict
|
||||
from dataclasses import dataclass
|
||||
from typing import Any, Dict, List, Optional, Set
|
||||
from typing import Any, Dict, List, Optional
|
||||
|
||||
import logging
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
# Bounded LRU cache for forum post to channel mappings.
|
||||
# OrderedDict evicts least-recently-used entries when full.
|
||||
_MAX_FORUM_CACHE = 10000
|
||||
_forum_post_cache: OrderedDict[str, str] = OrderedDict() # post_id -> channel_id
|
||||
|
||||
def register_forum_post(post_id: str, channel_id: str) -> None:
|
||||
"""Register a forum post's parent channel for hierarchical matching."""
|
||||
_forum_post_cache[post_id] = channel_id
|
||||
_forum_post_cache.move_to_end(post_id)
|
||||
while len(_forum_post_cache) > _MAX_FORUM_CACHE:
|
||||
_forum_post_cache.popitem(last=False)
|
||||
logger.debug("Registered forum post %s -> channel %s", post_id, channel_id)
|
||||
|
||||
|
||||
def resolve_forum_channel(post_id: str) -> Optional[str]:
|
||||
"""Get the parent channel ID for a forum post, if cached."""
|
||||
return _forum_post_cache.get(post_id)
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
class ProfileRoute:
|
||||
"""A single routing rule that maps a platform scope to a profile."""
|
||||
|
|
@ -103,8 +84,6 @@ class ProfileRoute:
|
|||
Supports hierarchical matching for Discord forums:
|
||||
- Direct channel match: chat_id == route.chat_id
|
||||
- Thread in channel: parent_chat_id == route.chat_id
|
||||
- Forum post: parent_chat_id is the forum post, check if post belongs to route's channel
|
||||
- Comment on forum post: parent_chat_id is the forum post, check hierarchy
|
||||
"""
|
||||
if not self.enabled:
|
||||
return False
|
||||
|
|
@ -121,12 +100,6 @@ class ProfileRoute:
|
|||
# Parent match (thread or direct child)
|
||||
if self.chat_id == parent_chat_id:
|
||||
return True
|
||||
# Forum post hierarchy: check if parent_chat_id is a forum post
|
||||
# that belongs to this channel
|
||||
if parent_chat_id:
|
||||
parent_channel = resolve_forum_channel(parent_chat_id)
|
||||
if parent_channel and self.chat_id == parent_channel:
|
||||
return True
|
||||
|
||||
# If chat_id was specified but didn't match any level, fail
|
||||
if self.chat_id:
|
||||
|
|
|
|||
|
|
@ -17342,16 +17342,50 @@ class GatewayRunner(GatewayAuthorizationMixin, GatewayKanbanWatchersMixin, Gatew
|
|||
fallback for sources that bypass ``build_source``.
|
||||
3. The active profile (the multiplexer's own home).
|
||||
"""
|
||||
from hermes_cli.profiles import get_active_profile_name, get_profile_dir
|
||||
from hermes_cli.profiles import (
|
||||
get_active_profile_name,
|
||||
get_profile_dir,
|
||||
profile_exists,
|
||||
)
|
||||
from hermes_constants import get_hermes_home
|
||||
|
||||
# Track whether a profile was explicitly requested (vs. falling back to default)
|
||||
explicit_profile = None
|
||||
try:
|
||||
name = (source.profile or "").strip()
|
||||
if name:
|
||||
explicit_profile = name # User explicitly set this profile
|
||||
if not name:
|
||||
name = self._profile_name_for_source(source)
|
||||
if name:
|
||||
explicit_profile = name # Routing explicitly set this profile
|
||||
if not name:
|
||||
name = get_active_profile_name() or "default"
|
||||
return get_profile_dir(name)
|
||||
|
||||
profile_dir = get_profile_dir(name)
|
||||
# Warn if an explicit profile doesn't exist on disk
|
||||
if explicit_profile and not profile_exists(name):
|
||||
logger.warning(
|
||||
"Profile %r does not exist for source %s/%s (guild_id=%s), "
|
||||
"falling back to global HERMES_HOME",
|
||||
explicit_profile,
|
||||
source.platform.value,
|
||||
source.chat_id,
|
||||
getattr(source, "guild_id", None),
|
||||
)
|
||||
return get_hermes_home()
|
||||
return profile_dir
|
||||
except Exception:
|
||||
from hermes_constants import get_hermes_home
|
||||
# Catch normalization errors, path errors, etc.
|
||||
logger.warning(
|
||||
"Failed to resolve profile directory for source %s/%s (guild_id=%s), "
|
||||
"falling back to global HERMES_HOME: %s",
|
||||
source.platform.value,
|
||||
source.chat_id,
|
||||
getattr(source, "guild_id", None),
|
||||
explicit_profile or "(no profile)",
|
||||
exc_info=True,
|
||||
)
|
||||
return get_hermes_home()
|
||||
|
||||
async def _run_agent_inner(
|
||||
|
|
|
|||
257
tests/gateway/test_profile_resolution.py
Normal file
257
tests/gateway/test_profile_resolution.py
Normal file
|
|
@ -0,0 +1,257 @@
|
|||
"""Tests for GatewayRunner._resolve_profile_home_for_source — profile resolution logic."""
|
||||
|
||||
import logging
|
||||
from pathlib import Path
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from gateway.session import SessionSource
|
||||
from gateway.run import GatewayRunner
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def mock_runner():
|
||||
"""Create a minimal mock GatewayRunner with the methods we need."""
|
||||
runner = MagicMock(spec=GatewayRunner)
|
||||
runner.config = MagicMock(profile_routes=[])
|
||||
# Bind the actual methods to the mock
|
||||
runner._profile_name_for_source = GatewayRunner._profile_name_for_source.__get__(runner)
|
||||
runner._resolve_profile_home_for_source = GatewayRunner._resolve_profile_home_for_source.__get__(runner)
|
||||
return runner
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def discord_source():
|
||||
"""Create a basic Discord SessionSource for testing."""
|
||||
return SessionSource(
|
||||
platform=MagicMock(value="discord"),
|
||||
chat_id="123456",
|
||||
guild_id="789",
|
||||
thread_id=None,
|
||||
parent_chat_id=None,
|
||||
)
|
||||
|
||||
|
||||
class TestResolutionOrder:
|
||||
"""Tests that profile resolution follows the correct priority order."""
|
||||
|
||||
def test_source_profile_wins_over_routing(self, mock_runner, discord_source):
|
||||
"""source.profile should be used even if routing would match."""
|
||||
discord_source.profile = "from-source"
|
||||
|
||||
with patch("hermes_cli.profiles.get_active_profile_name", return_value="active"):
|
||||
with patch("hermes_cli.profiles.get_profile_dir") as mock_get_dir:
|
||||
with patch("hermes_cli.profiles.profile_exists", return_value=True):
|
||||
mock_get_dir.return_value = Path("/hermes/profiles/from-source")
|
||||
result = mock_runner._resolve_profile_home_for_source(discord_source)
|
||||
|
||||
assert result == Path("/hermes/profiles/from-source")
|
||||
mock_get_dir.assert_called_once_with("from-source")
|
||||
|
||||
def test_routing_wins_over_active_profile(self, mock_runner, discord_source):
|
||||
"""When source.profile is empty, routing should win over active profile."""
|
||||
discord_source.profile = None
|
||||
|
||||
# Mock routing to return a profile
|
||||
with patch("hermes_cli.profiles.get_active_profile_name", return_value="active"):
|
||||
with patch("hermes_cli.profiles.get_profile_dir") as mock_get_dir:
|
||||
with patch("hermes_cli.profiles.profile_exists", return_value=True):
|
||||
mock_get_dir.return_value = Path("/hermes/profiles/routed")
|
||||
|
||||
# Manually set routing to return a profile
|
||||
mock_runner._profile_name_for_source = MagicMock(return_value="routed")
|
||||
|
||||
result = mock_runner._resolve_profile_home_for_source(discord_source)
|
||||
|
||||
assert result == Path("/hermes/profiles/routed")
|
||||
mock_get_dir.assert_called_once_with("routed")
|
||||
|
||||
def test_active_profile_fallback(self, mock_runner, discord_source):
|
||||
"""When source.profile and routing both return None, active profile is used."""
|
||||
discord_source.profile = None
|
||||
|
||||
with patch("hermes_cli.profiles.get_active_profile_name", return_value="active"):
|
||||
with patch("hermes_cli.profiles.get_profile_dir") as mock_get_dir:
|
||||
mock_get_dir.return_value = Path("/hermes/profiles/active")
|
||||
|
||||
# No routing match
|
||||
mock_runner._profile_name_for_source = MagicMock(return_value=None)
|
||||
|
||||
result = mock_runner._resolve_profile_home_for_source(discord_source)
|
||||
|
||||
assert result == Path("/hermes/profiles/active")
|
||||
mock_get_dir.assert_called_once_with("active")
|
||||
|
||||
def test_default_fallback_when_no_active(self, mock_runner, discord_source):
|
||||
"""When even active profile is None, 'default' is used."""
|
||||
discord_source.profile = None
|
||||
|
||||
with patch("hermes_cli.profiles.get_active_profile_name", return_value=None):
|
||||
with patch("hermes_cli.profiles.get_profile_dir") as mock_get_dir:
|
||||
mock_get_dir.return_value = Path("/hermes")
|
||||
|
||||
mock_runner._profile_name_for_source = MagicMock(return_value=None)
|
||||
|
||||
result = mock_runner._resolve_profile_home_for_source(discord_source)
|
||||
|
||||
assert result == Path("/hermes")
|
||||
mock_get_dir.assert_called_once_with("default")
|
||||
|
||||
|
||||
class TestMissingProfileWarning:
|
||||
"""Tests for warning when a profile doesn't exist on disk."""
|
||||
|
||||
def test_nonexistent_profile_warning(self, mock_runner, discord_source, caplog):
|
||||
"""When source.profile points to a nonexistent profile, log a WARNING."""
|
||||
discord_source.profile = "nonexistent"
|
||||
|
||||
with patch("hermes_cli.profiles.get_active_profile_name", return_value="active"):
|
||||
with patch("hermes_cli.profiles.get_profile_dir") as mock_get_dir:
|
||||
mock_get_dir.return_value = Path("/hermes/profiles/nonexistent")
|
||||
with patch("hermes_cli.profiles.profile_exists", return_value=False):
|
||||
with patch("hermes_constants.get_hermes_home", return_value=Path("/hermes")):
|
||||
with caplog.at_level(logging.WARNING):
|
||||
result = mock_runner._resolve_profile_home_for_source(discord_source)
|
||||
|
||||
# Should fall back to global HERMES_HOME
|
||||
assert result == Path("/hermes")
|
||||
|
||||
# Should have logged a warning
|
||||
assert len(caplog.records) == 1
|
||||
assert caplog.records[0].levelname == "WARNING"
|
||||
assert "nonexistent" in caplog.records[0].message
|
||||
assert "does not exist" in caplog.records[0].message
|
||||
assert "discord" in caplog.records[0].message
|
||||
assert "123456" in caplog.records[0].message
|
||||
|
||||
def test_nonexistent_routing_profile_warning(self, mock_runner, discord_source, caplog):
|
||||
"""When routing returns a nonexistent profile, log a WARNING."""
|
||||
discord_source.profile = None
|
||||
|
||||
with patch("hermes_cli.profiles.get_active_profile_name", return_value="active"):
|
||||
with patch("hermes_cli.profiles.get_profile_dir") as mock_get_dir:
|
||||
mock_get_dir.return_value = Path("/hermes/profiles/routed")
|
||||
with patch("hermes_cli.profiles.profile_exists", return_value=False):
|
||||
with patch("hermes_constants.get_hermes_home", return_value=Path("/hermes")):
|
||||
# Routing returns a profile that doesn't exist
|
||||
mock_runner._profile_name_for_source = MagicMock(return_value="routed")
|
||||
|
||||
with caplog.at_level(logging.WARNING):
|
||||
result = mock_runner._resolve_profile_home_for_source(discord_source)
|
||||
|
||||
# Should fall back to global HERMES_HOME
|
||||
assert result == Path("/hermes")
|
||||
|
||||
# Should have logged a warning
|
||||
assert len(caplog.records) == 1
|
||||
assert "routed" in caplog.records[0].message
|
||||
|
||||
def test_empty_source_profile_no_warning(self, mock_runner, discord_source, caplog):
|
||||
"""When source.profile is empty, silent fallback to active profile (no warning)."""
|
||||
discord_source.profile = None
|
||||
|
||||
with patch("hermes_cli.profiles.get_active_profile_name", return_value="active"):
|
||||
with patch("hermes_cli.profiles.get_profile_dir") as mock_get_dir:
|
||||
mock_get_dir.return_value = Path("/hermes/profiles/active")
|
||||
with patch("hermes_cli.profiles.profile_exists", return_value=True):
|
||||
with caplog.at_level(logging.WARNING):
|
||||
mock_runner._profile_name_for_source = MagicMock(return_value=None)
|
||||
|
||||
result = mock_runner._resolve_profile_home_for_source(discord_source)
|
||||
|
||||
# Should use active profile
|
||||
assert result == Path("/hermes/profiles/active")
|
||||
|
||||
# No warnings (active profile exists)
|
||||
assert not any(r.levelname == "WARNING" for r in caplog.records)
|
||||
|
||||
def test_existing_profile_no_warning(self, mock_runner, discord_source, caplog):
|
||||
"""When the profile exists, no warning should be logged."""
|
||||
discord_source.profile = "existing"
|
||||
|
||||
with patch("hermes_cli.profiles.get_active_profile_name", return_value="active"):
|
||||
with patch("hermes_cli.profiles.get_profile_dir") as mock_get_dir:
|
||||
mock_get_dir.return_value = Path("/hermes/profiles/existing")
|
||||
with patch("hermes_cli.profiles.profile_exists", return_value=True):
|
||||
with caplog.at_level(logging.WARNING):
|
||||
result = mock_runner._resolve_profile_home_for_source(discord_source)
|
||||
|
||||
assert result == Path("/hermes/profiles/existing")
|
||||
|
||||
# No warnings
|
||||
assert not any(r.levelname == "WARNING" for r in caplog.records)
|
||||
|
||||
|
||||
class TestExceptionHandling:
|
||||
"""Tests for exception handling in profile resolution."""
|
||||
|
||||
def test_get_profile_dir_exception_logs_warning(self, mock_runner, discord_source, caplog):
|
||||
"""When get_profile_dir raises an exception, log a WARNING with context."""
|
||||
discord_source.profile = "bad-profile"
|
||||
|
||||
with patch("hermes_cli.profiles.get_active_profile_name", return_value="active"):
|
||||
with patch("hermes_cli.profiles.get_profile_dir", side_effect=ValueError("Invalid profile name")):
|
||||
with patch("hermes_constants.get_hermes_home", return_value=Path("/hermes")):
|
||||
with caplog.at_level(logging.WARNING):
|
||||
result = mock_runner._resolve_profile_home_for_source(discord_source)
|
||||
|
||||
# Should fall back to global HERMES_HOME
|
||||
assert result == Path("/hermes")
|
||||
|
||||
# Should have logged a warning with exception info
|
||||
assert len(caplog.records) == 1
|
||||
assert caplog.records[0].levelname == "WARNING"
|
||||
assert "bad-profile" in caplog.records[0].message
|
||||
assert "Failed to resolve profile directory" in caplog.records[0].message
|
||||
|
||||
def test_exception_with_no_profile_name(self, mock_runner, discord_source, caplog):
|
||||
"""Exception when no profile was set should still log a warning."""
|
||||
discord_source.profile = None
|
||||
|
||||
with patch("hermes_cli.profiles.get_active_profile_name", return_value=None):
|
||||
with patch("hermes_cli.profiles.get_profile_dir", side_effect=RuntimeError("Filesystem error")):
|
||||
with patch("hermes_constants.get_hermes_home", return_value=Path("/hermes")):
|
||||
mock_runner._profile_name_for_source = MagicMock(return_value=None)
|
||||
|
||||
with caplog.at_level(logging.WARNING):
|
||||
result = mock_runner._resolve_profile_home_for_source(discord_source)
|
||||
|
||||
assert result == Path("/hermes")
|
||||
|
||||
# Warning should mention "(no profile)"
|
||||
assert "(no profile)" in caplog.records[0].message
|
||||
|
||||
|
||||
class TestRoutingConsultation:
|
||||
"""Tests that _profile_name_for_source is consulted when source.profile is empty."""
|
||||
|
||||
def test_routing_consulted_when_source_profile_empty(self, mock_runner, discord_source):
|
||||
"""_profile_name_for_source should be called when source.profile is empty."""
|
||||
discord_source.profile = None
|
||||
|
||||
with patch("hermes_cli.profiles.get_active_profile_name", return_value="active"):
|
||||
with patch("hermes_cli.profiles.get_profile_dir") as mock_get_dir:
|
||||
mock_get_dir.return_value = Path("/hermes/profiles/routed")
|
||||
|
||||
mock_runner._profile_name_for_source = MagicMock(return_value="routed")
|
||||
|
||||
mock_runner._resolve_profile_home_for_source(discord_source)
|
||||
|
||||
# Should have called routing
|
||||
mock_runner._profile_name_for_source.assert_called_once_with(discord_source)
|
||||
|
||||
def test_routing_not_consulted_when_source_profile_set(self, mock_runner, discord_source):
|
||||
"""_profile_name_for_source should NOT be called when source.profile is set."""
|
||||
discord_source.profile = "from-source"
|
||||
|
||||
with patch("hermes_cli.profiles.get_active_profile_name", return_value="active"):
|
||||
with patch("hermes_cli.profiles.get_profile_dir") as mock_get_dir:
|
||||
mock_get_dir.return_value = Path("/hermes/profiles/from-source")
|
||||
|
||||
mock_runner._profile_name_for_source = MagicMock(return_value="routed")
|
||||
|
||||
mock_runner._resolve_profile_home_for_source(discord_source)
|
||||
|
||||
# Should NOT have called routing
|
||||
mock_runner._profile_name_for_source.assert_not_called()
|
||||
|
|
@ -215,3 +215,32 @@ class TestParentChatIdMatching:
|
|||
r = ProfileRoute(name="g", platform="discord", profile="server",
|
||||
guild_id="111")
|
||||
assert r.matches("discord", guild_id="111", chat_id="333", parent_chat_id="444")
|
||||
|
||||
|
||||
class TestForumPostMatching:
|
||||
"""Test that forum posts match via parent_chat_id (direct parent)."""
|
||||
|
||||
def test_forum_channel_route_matches_forum_post(self):
|
||||
"""A route on a forum channel should match comments on posts in that forum.
|
||||
|
||||
In Discord, forum posts (threads) have parent_chat_id = forum channel ID.
|
||||
No cache is needed — the parent relationship is direct.
|
||||
"""
|
||||
r = ProfileRoute(name="forum", platform="discord", profile="forum_profile",
|
||||
chat_id="forum_channel_123")
|
||||
# A comment on a forum post: chat_id=post_thread_id, parent_chat_id=forum_channel_id
|
||||
assert r.matches("discord", chat_id="post_thread_456", parent_chat_id="forum_channel_123")
|
||||
|
||||
def test_forum_post_comment_matches_channel_not_thread_id(self):
|
||||
"""Verify that thread_id matching is distinct from parent_chat_id matching."""
|
||||
routes = [
|
||||
ProfileRoute(name="forum", platform="discord", profile="forum_profile",
|
||||
chat_id="forum_channel_123"),
|
||||
ProfileRoute(name="post", platform="discord", profile="post_profile",
|
||||
thread_id="post_thread_456"),
|
||||
]
|
||||
# A comment on the forum post should match the forum channel route, not the thread route
|
||||
m = match_profile_route(routes, "discord", chat_id="post_thread_456",
|
||||
parent_chat_id="forum_channel_123")
|
||||
assert m is not None
|
||||
assert m.profile == "forum_profile"
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue