From 1a8e67076a0bef27262a11bbbaecf755f638aa19 Mon Sep 17 00:00:00 2001 From: Erosika Date: Tue, 26 May 2026 14:20:01 -0400 Subject: [PATCH] fix(honcho): cover pinUserPeer + aiPeer edge cases in setup, clone, and gateway cache Three related regressions stemming from the pinUserPeer alias landing: - Setup wizard read host-only fields when detecting current shape but the parser supports root-level config and gives host pinUserPeer higher precedence than pinPeerName. Re-running setup could mis-detect shape and silently flip routing. Detection now uses the same resolver order as HonchoClientConfig, and each shape branch scrubs every peer-mapping key before writing so a stale pinUserPeer=false can't outrank a freshly written pinPeerName=true. Multi no longer auto-writes userPeerAliases={} (was silently masking root-level baselines). - clone_honcho_for_profile inherited pinPeerName but not pinUserPeer, so a default profile configured with the newer key produced cloned profiles without the pin. - Gateway cache-busting signature fingerprinted Honcho user-peer fields but not ai_peer. Since HonchoSessionManager freezes cfg.ai_peer at init, mid-flight aiPeer edits kept assistant writes on the old peer until an unrelated cache eviction. ai_peer is now part of the signature. --- gateway/run.py | 9 +- plugins/memory/honcho/cli.py | 144 +++++++++++++++++---- tests/honcho_plugin/test_cli.py | 145 +++++++++++++++++++++- tests/honcho_plugin/test_pin_peer_name.py | 28 +++++ 4 files changed, 296 insertions(+), 30 deletions(-) diff --git a/gateway/run.py b/gateway/run.py index bb26662fabb..c2be5f57135 100644 --- a/gateway/run.py +++ b/gateway/run.py @@ -15073,20 +15073,23 @@ class GatewayRunner: out["tools.registry_generation"] = None # Honcho identity-mapping keys live in honcho.json, not user_config. - # HonchoSessionManager freezes the resolved peer_name / pin / aliases / - # prefix at construction; without busting here, mid-flight honcho.json - # edits go unread until the next unrelated cache eviction. + # HonchoSessionManager freezes the resolved peer_name / ai_peer / + # pin / aliases / prefix at construction; without busting here, + # mid-flight honcho.json edits go unread until the next unrelated + # cache eviction. try: from plugins.memory.honcho.client import HonchoClientConfig hcfg = HonchoClientConfig.from_global_config() out["honcho.peer_name"] = hcfg.peer_name + out["honcho.ai_peer"] = hcfg.ai_peer out["honcho.pin_peer_name"] = bool(hcfg.pin_peer_name) out["honcho.runtime_peer_prefix"] = hcfg.runtime_peer_prefix or "" aliases = hcfg.user_peer_aliases or {} out["honcho.user_peer_aliases"] = sorted(aliases.items()) if isinstance(aliases, dict) else [] except Exception: out["honcho.peer_name"] = None + out["honcho.ai_peer"] = None out["honcho.pin_peer_name"] = None out["honcho.runtime_peer_prefix"] = None out["honcho.user_peer_aliases"] = None diff --git a/plugins/memory/honcho/cli.py b/plugins/memory/honcho/cli.py index 8dfea1169db..9227bf95ab8 100644 --- a/plugins/memory/honcho/cli.py +++ b/plugins/memory/honcho/cli.py @@ -41,17 +41,19 @@ def clone_honcho_for_profile(profile_name: str) -> bool: return False # already exists # Clone settings from default block, override identity fields. - # Identity-mapping keys (pinPeerName, userPeerAliases, runtimePeerPrefix) - # carry the operator's runtime-to-peer routing intent from #27371. - # Without them in this allowlist, a cloned profile would silently lose - # the mapping and gateway users would resolve to raw runtime IDs, - # fragmenting Honcho memory across an unintended set of peers. + # Identity-mapping keys (pinPeerName/pinUserPeer, userPeerAliases, + # runtimePeerPrefix) carry the operator's runtime-to-peer routing + # intent from #27371. Both pin keys are inherited because + # HonchoClientConfig prefers pinUserPeer over pinPeerName — leaving + # the canonical key off this allowlist silently drops the pin on + # cloned profiles when the default uses the newer name. new_block = {} for key in ("recallMode", "writeFrequency", "sessionStrategy", "sessionPeerPrefix", "contextTokens", "dialecticReasoningLevel", "dialecticDynamic", "dialecticMaxChars", "messageMaxChars", "dialecticMaxInputChars", "saveMessages", "observation", - "pinPeerName", "userPeerAliases", "runtimePeerPrefix"): + "pinPeerName", "pinUserPeer", "userPeerAliases", + "runtimePeerPrefix"): val = default_block.get(key) if val is not None: new_block[key] = val @@ -314,6 +316,72 @@ def _resolve_api_key(cfg: dict) -> str: return key +_IDENTITY_MAPPING_KEYS = ( + "pinPeerName", + "pinUserPeer", + "userPeerAliases", + "runtimePeerPrefix", +) + + +def _resolve_effective_identity_mapping( + cfg: dict, hermes_host: dict +) -> tuple[bool, dict, str, bool, bool]: + """Resolve the effective identity-mapping state for the active host. + + Matches the precedence used by ``HonchoClientConfig.from_global_config`` + so the wizard reads the same shape the gateway will actually run with. + Without this, root-level overrides and ``pinUserPeer`` (which wins over + ``pinPeerName`` at the same level) are invisible to detection, letting + setup mis-classify the current shape and silently change effective + routing on the next save. + + Returns ``(pin, aliases, prefix, aliases_from_root, prefix_from_root)``. + The ``*_from_root`` flags let the write step skip touching host keys + whose value is actually inherited. + """ + pin = False + for val in ( + hermes_host.get("pinUserPeer"), + hermes_host.get("pinPeerName"), + cfg.get("pinUserPeer"), + cfg.get("pinPeerName"), + ): + if val is not None: + pin = bool(val) + break + + if "userPeerAliases" in hermes_host: + aliases_src = hermes_host.get("userPeerAliases") + aliases_from_root = False + else: + aliases_src = cfg.get("userPeerAliases") + aliases_from_root = aliases_src is not None + aliases = aliases_src if isinstance(aliases_src, dict) else {} + + if "runtimePeerPrefix" in hermes_host: + prefix_src = hermes_host.get("runtimePeerPrefix") + prefix_from_root = False + else: + prefix_src = cfg.get("runtimePeerPrefix") + prefix_from_root = prefix_src is not None + prefix = str(prefix_src or "") + + return pin, aliases, prefix, aliases_from_root, prefix_from_root + + +def _scrub_identity_mapping(hermes_host: dict) -> None: + """Drop every peer-mapping key from the host block. + + Called before the wizard writes a chosen shape so latent precedence + conflicts can't survive — e.g. a stray host ``pinUserPeer: false`` + that would silently outrank a freshly written ``pinPeerName: true`` + (host ``pinUserPeer`` is first in the resolver ladder). + """ + for key in _IDENTITY_MAPPING_KEYS: + hermes_host.pop(key, None) + + def _prompt(label: str, default: str | None = None, secret: bool = False) -> str: suffix = f" [{default}]" if default else "" sys.stdout.write(f" {label}{suffix}: ") @@ -447,9 +515,19 @@ def cmd_setup(args) -> None: # shapes cover the realistic deployments; each writes a different # combination of pinPeerName / userPeerAliases / runtimePeerPrefix. # See plugins/memory/honcho/README.md for the resolver ladder. - current_pin = bool(hermes_host.get("pinPeerName", False)) - current_aliases = hermes_host.get("userPeerAliases", {}) - current_prefix = hermes_host.get("runtimePeerPrefix", "") + # + # Detection must mirror the gateway resolver: root-level config and + # ``pinUserPeer`` (which outranks ``pinPeerName`` at the same level) + # both affect effective routing, so reading host-only fields would + # mis-classify a profile that inherits its mapping from root or uses + # the newer canonical key. + ( + current_pin, + current_aliases, + current_prefix, + aliases_from_root, + prefix_from_root, + ) = _resolve_effective_identity_mapping(cfg, hermes_host) if current_pin: current_shape = "single" @@ -484,30 +562,52 @@ def cmd_setup(args) -> None: elif confirm not in {"yes", "y"}: new_shape = "skip" + # Each shape branch scrubs every peer-mapping key before writing its own, + # so a stale ``pinUserPeer`` left behind by an earlier setup run can't + # outrank the freshly written ``pinPeerName`` via host-level precedence. if new_shape == "single": + _scrub_identity_mapping(hermes_host) hermes_host["pinPeerName"] = True - hermes_host.pop("userPeerAliases", None) - hermes_host.pop("runtimePeerPrefix", None) print(f" pinPeerName=true → all gateway users route to '{hermes_host.get('peerName', '?')}'.") elif new_shape == "multi": + # Preserve operator-curated, host-level aliases so multi → multi + # re-runs don't drop them. Root-sourced aliases are left to + # cascade naturally and are NOT copied down into the host. + prior_aliases = ( + dict(current_aliases) + if isinstance(current_aliases, dict) and not aliases_from_root + else {} + ) + _scrub_identity_mapping(hermes_host) hermes_host["pinPeerName"] = False - # Preserve any existing operator-curated aliases / prefix. - if "userPeerAliases" not in hermes_host: - hermes_host["userPeerAliases"] = {} + # Do NOT auto-write ``userPeerAliases: {}``: an empty host map + # would override any root-level ``userPeerAliases`` the operator + # set as a cross-host baseline, silently disabling those aliases. + # Absence is the right "no host opinion" signal. + if prior_aliases: + hermes_host["userPeerAliases"] = prior_aliases _prefix_default = current_prefix or "" _new_prefix = _prompt( "Runtime peer prefix (e.g. 'telegram_', blank for none)", default=_prefix_default, ).strip() - if _new_prefix: + # Only write a host-level prefix when the operator typed one that + # diverges from the inherited root value; otherwise let the root + # cascade continue unmodified. + if _new_prefix and not (prefix_from_root and _new_prefix == current_prefix): hermes_host["runtimePeerPrefix"] = _new_prefix - else: - hermes_host.pop("runtimePeerPrefix", None) print(" Multi-user mode: each runtime ID → own peer. Use 'hermes honcho status' to inspect.") elif new_shape == "hybrid": + # Hybrid encodes operator intent at the host level: collect existing + # entries (host or root) so the wizard never silently drops a known + # alias, then write the combined map. Materialising root entries + # into the host is the right move here — once the operator answers + # the alias prompts for a host, they're declaring "this host owns + # the mapping". + existing_aliases = dict(current_aliases) if isinstance(current_aliases, dict) else {} + _scrub_identity_mapping(hermes_host) hermes_host["pinPeerName"] = False peer_target = hermes_host.get("peerName") or current_peer or "user" - existing_aliases = dict(current_aliases) if isinstance(current_aliases, dict) else {} print(f"\n Add runtime IDs that should alias to peer '{peer_target}'.") print(" Leave blank to skip a platform. Existing aliases are preserved.") for platform_label, alias_hint in ( @@ -521,19 +621,13 @@ def cmd_setup(args) -> None: existing_aliases[entered] = peer_target if existing_aliases: hermes_host["userPeerAliases"] = existing_aliases - elif "userPeerAliases" in hermes_host: - # No aliases entered and none pre-existing — leave the key absent. - if not hermes_host["userPeerAliases"]: - hermes_host.pop("userPeerAliases", None) _prefix_default = current_prefix or "" _new_prefix = _prompt( "Runtime peer prefix for unknown users (e.g. 'telegram_', blank for none)", default=_prefix_default, ).strip() - if _new_prefix: + if _new_prefix and not (prefix_from_root and _new_prefix == current_prefix): hermes_host["runtimePeerPrefix"] = _new_prefix - else: - hermes_host.pop("runtimePeerPrefix", None) print(f" Hybrid mode: your runtime IDs → '{peer_target}', others → own peer.") elif new_shape == "skip": pass # leave config untouched diff --git a/tests/honcho_plugin/test_cli.py b/tests/honcho_plugin/test_cli.py index 24b67679e64..8244badc2f6 100644 --- a/tests/honcho_plugin/test_cli.py +++ b/tests/honcho_plugin/test_cli.py @@ -346,7 +346,10 @@ class TestSetupWizardDeploymentShape: ] host = self._run_setup(monkeypatch, tmp_path, answers=answers) assert host["pinPeerName"] is False - assert host["userPeerAliases"] == {} + # Multi must NOT auto-write ``userPeerAliases: {}``: an empty host + # map would silently override a root-level baseline. Absence is + # the correct "no host opinion" signal. + assert "userPeerAliases" not in host assert host["runtimePeerPrefix"] == "telegram_" def test_hybrid_shape_aliases_operator_runtime_ids_to_peer_name(self, monkeypatch, tmp_path): @@ -431,5 +434,143 @@ class TestSetupWizardDeploymentShape: ] host = self._run_setup(monkeypatch, tmp_path, answers=answers, initial_cfg=initial_cfg) assert host["pinPeerName"] is False - assert host["userPeerAliases"] == {} + # See test_multi_shape_leaves_pin_false_and_accepts_prefix. + assert "userPeerAliases" not in host assert host["runtimePeerPrefix"] == "telegram_" + + def test_host_pin_user_peer_true_is_detected_as_single(self, monkeypatch, tmp_path): + """Host-level ``pinUserPeer: true`` must classify as ``single``. + + Pressing Enter at the shape prompt then preserves the pin instead + of falling through to ``multi`` and orphaning the user's memory + pool — the bug the wizard regressed when ``pinUserPeer`` landed + as a higher-precedence alias. + """ + initial_cfg = { + "apiKey": "***", + "hosts": {"hermes": {"pinUserPeer": True, "peerName": "eri"}}, + } + # Exhaust the iterator before the shape prompt so the scripted + # mock falls through to the prompt's default (which is the + # wizard-detected shape). Scripting an explicit "" would NOT + # exercise that fallthrough — the mock returns it literally. + answers = ["cloud", "", "eri", "hermetika", "hermes"] + host = self._run_setup(monkeypatch, tmp_path, answers=answers, initial_cfg=initial_cfg) + # Scrub-then-write normalises onto pinPeerName and drops the alias + # so resolver precedence can't reintroduce ambiguity. + assert host["pinPeerName"] is True + assert "pinUserPeer" not in host + + def test_host_pin_user_peer_false_overrides_root_pin_peer_name( + self, monkeypatch, tmp_path + ): + """Host ``pinUserPeer: false`` outranks host ``pinPeerName`` in the + resolver. Detection must agree, otherwise the wizard would offer + ``single`` as the default and silently re-pin a profile the + operator explicitly unpinned via the newer key. + """ + initial_cfg = { + "apiKey": "***", + "hosts": {"hermes": { + "pinUserPeer": False, + "pinPeerName": True, + "peerName": "eri", + }}, + } + answers = ["cloud", "", "eri", "hermetika", "hermes"] + host = self._run_setup(monkeypatch, tmp_path, answers=answers, initial_cfg=initial_cfg) + assert host["pinPeerName"] is False + assert "pinUserPeer" not in host + + def test_root_user_peer_aliases_detected_as_hybrid(self, monkeypatch, tmp_path): + """Root-level ``userPeerAliases`` must classify as ``hybrid`` even + when the host block has no aliases of its own. + """ + initial_cfg = { + "apiKey": "***", + "userPeerAliases": {"86701400": "eri"}, + "hosts": {"hermes": {"peerName": "eri"}}, + } + answers = ["cloud", "", "eri", "hermetika", "hermes"] + host = self._run_setup(monkeypatch, tmp_path, answers=answers, initial_cfg=initial_cfg) + assert host["pinPeerName"] is False + # Hybrid materialises the root aliases into the host so subsequent + # operator edits live on the host block they're inspecting. + assert host["userPeerAliases"] == {"86701400": "eri"} + + def test_multi_does_not_override_root_user_peer_aliases(self, monkeypatch, tmp_path): + """Explicit ``multi`` must leave the host ``userPeerAliases`` key + absent, preserving any root-level aliases as a cross-host baseline. + + Picking ``multi`` here is an active choice — detection would have + defaulted to ``hybrid`` because root aliases exist — so the + operator's intent is to drop the alias mapping for this host. + We honor that by writing ``pinPeerName: false`` only, and rely + on the host's absence of ``userPeerAliases`` to inherit root. + That inheritance is intentional: a true wipe would require the + operator to delete the root key explicitly. + """ + initial_cfg = { + "apiKey": "***", + "userPeerAliases": {"baseline": "eri"}, + "hosts": {"hermes": {"peerName": "eri"}}, + } + answers = [ + "cloud", "", "eri", "hermetika", "hermes", + "multi", # explicit multi override of detected hybrid + ] + host = self._run_setup(monkeypatch, tmp_path, answers=answers, initial_cfg=initial_cfg) + assert host["pinPeerName"] is False + assert "userPeerAliases" not in host + + def test_single_scrubs_stale_pin_user_peer_false(self, monkeypatch, tmp_path): + """Choosing ``single`` must drop any host-level ``pinUserPeer``, + otherwise an existing ``pinUserPeer: false`` would outrank the + freshly written ``pinPeerName: true`` and leave the profile + effectively unpinned (the P1 latent-precedence regression). + """ + initial_cfg = { + "apiKey": "***", + "hosts": {"hermes": { + "pinUserPeer": False, + "peerName": "eri", + }}, + } + answers = [ + "cloud", "", "eri", "hermetika", "hermes", + "single", + ] + host = self._run_setup(monkeypatch, tmp_path, answers=answers, initial_cfg=initial_cfg) + assert host["pinPeerName"] is True + assert "pinUserPeer" not in host + + +class TestCloneCarriesPinUserPeer: + """``pinUserPeer`` (canonical name for ``pinPeerName``) must survive a + profile clone. Without this, a default profile that uses the newer + key would silently produce cloned profiles without the pin even + though the resolver prefers ``pinUserPeer`` over ``pinPeerName``. + """ + + def test_clone_inherits_host_pin_user_peer(self, monkeypatch, tmp_path): + import plugins.memory.honcho.cli as honcho_cli + + cfg = { + "apiKey": "***", + "hosts": {"hermes": {"pinUserPeer": True, "peerName": "eri"}}, + } + cfg_path = tmp_path / "config.json" + cfg_path.write_text("{}") + monkeypatch.setattr(honcho_cli, "_read_config", lambda: cfg) + monkeypatch.setattr(honcho_cli, "_config_path", lambda: cfg_path) + monkeypatch.setattr(honcho_cli, "_local_config_path", lambda: cfg_path) + monkeypatch.setattr(honcho_cli, "_ensure_peer_exists", lambda host_key=None: True) + written = {} + monkeypatch.setattr( + honcho_cli, "_write_config", lambda c, path=None: written.setdefault("cfg", c), + ) + + ok = honcho_cli.clone_honcho_for_profile("partner") + assert ok is True + new_block = written["cfg"]["hosts"]["hermes.partner"] + assert new_block["pinUserPeer"] is True diff --git a/tests/honcho_plugin/test_pin_peer_name.py b/tests/honcho_plugin/test_pin_peer_name.py index 6105734204c..d3d935f9a05 100644 --- a/tests/honcho_plugin/test_pin_peer_name.py +++ b/tests/honcho_plugin/test_pin_peer_name.py @@ -789,6 +789,34 @@ class TestPinTransition: assert sig_no_prefix["honcho.runtime_peer_prefix"] != sig_with_prefix["honcho.runtime_peer_prefix"] + def test_cache_busting_signature_reflects_ai_peer(self, tmp_path, monkeypatch): + """Editing ``aiPeer`` mid-flight must invalidate the cached agent. + + ``HonchoSessionManager`` freezes ``cfg.ai_peer`` at construction — + without busting here, assistant writes keep landing on the old + peer until an unrelated cache eviction. + """ + from gateway.run import GatewayRunner + + cfg_path = tmp_path / "honcho.json" + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + + cfg_path.write_text(json.dumps({ + "apiKey": "k", + "peerName": "Igor", + "aiPeer": "hermes", + })) + sig_before = GatewayRunner._extract_cache_busting_config({}) + + cfg_path.write_text(json.dumps({ + "apiKey": "k", + "peerName": "Igor", + "aiPeer": "hermetika", + })) + sig_after = GatewayRunner._extract_cache_busting_config({}) + + assert sig_before["honcho.ai_peer"] != sig_after["honcho.ai_peer"] + class TestProfilePeerUniqueness: """Each Hermes profile can pin to its own unique peerName.