diff --git a/plugins/memory/honcho/cli.py b/plugins/memory/honcho/cli.py index 092b7c823d3..bd74f42abd2 100644 --- a/plugins/memory/honcho/cli.py +++ b/plugins/memory/honcho/cli.py @@ -41,22 +41,20 @@ 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/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. + # Identity-mapping keys (pinUserPeer, userPeerAliases, runtimePeerPrefix) + # carry the operator's runtime-to-peer routing intent from #27371. new_block = {} for key in ("recallMode", "writeFrequency", "sessionStrategy", "sessionPeerPrefix", "contextTokens", "dialecticReasoningLevel", "dialecticDynamic", "dialecticMaxChars", "messageMaxChars", "dialecticMaxInputChars", "saveMessages", "observation", - "pinPeerName", "pinUserPeer", "userPeerAliases", - "runtimePeerPrefix"): + "pinUserPeer", "userPeerAliases", "runtimePeerPrefix"): val = default_block.get(key) if val is not None: new_block[key] = val + # Carry a legacy default-block pinPeerName forward under the canonical key. + if "pinUserPeer" not in new_block and default_block.get("pinPeerName") is not None: + new_block["pinUserPeer"] = default_block["pinPeerName"] # Inherit peer name from default peer_name = default_block.get("peerName") or cfg.get("peerName") @@ -371,15 +369,28 @@ def _resolve_effective_identity_mapping( 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). + Called before the wizard writes a chosen shape so a stale alias, prefix, + or pin from an earlier run can't bleed into the new mapping. """ for key in _IDENTITY_MAPPING_KEYS: hermes_host.pop(key, None) +def _migrate_pin_key(block: dict) -> bool: + """Rewrite a legacy ``pinPeerName`` to canonical ``pinUserPeer`` in place. + + ``pinUserPeer`` wins over ``pinPeerName`` in the resolver, so setup writes + only the canonical form and migrates on touch to stop configs carrying + both. Returns True if the block changed. + """ + if "pinPeerName" not in block: + return False + legacy = block.pop("pinPeerName") + if "pinUserPeer" not in block: + block["pinUserPeer"] = legacy + return True + + def _prompt(label: str, default: str | None = None, secret: bool = False) -> str: suffix = f" [{default}]" if default else "" sys.stdout.write(f" {label}{suffix}: ") @@ -446,6 +457,10 @@ def cmd_setup(args) -> None: hosts = cfg.setdefault("hosts", {}) hermes_host = hosts.setdefault(_host_key(), {}) + # Canonicalize any legacy pinPeerName before detection/writes. + _migrate_pin_key(cfg) + _migrate_pin_key(hermes_host) + # --- 1. Cloud or local? --- print(" Deployment:") print(" cloud -- Honcho cloud (api.honcho.dev)") @@ -599,12 +614,11 @@ def cmd_setup(args) -> None: 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. + # so a stale alias/prefix/pin from an earlier run starts clean. if new_shape == "single": _scrub_identity_mapping(hermes_host) - hermes_host["pinPeerName"] = True - print(f" pinPeerName=true → all gateway users route to '{hermes_host.get('peerName', '?')}'.") + hermes_host["pinUserPeer"] = True + print(f" pinUserPeer=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 @@ -615,7 +629,7 @@ def cmd_setup(args) -> None: else {} ) _scrub_identity_mapping(hermes_host) - hermes_host["pinPeerName"] = False + hermes_host["pinUserPeer"] = False # 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. @@ -642,7 +656,7 @@ def cmd_setup(args) -> None: # the mapping". existing_aliases = dict(current_aliases) if isinstance(current_aliases, dict) else {} _scrub_identity_mapping(hermes_host) - hermes_host["pinPeerName"] = False + hermes_host["pinUserPeer"] = False peer_target = hermes_host.get("peerName") or current_peer or "user" print(f"\n Add runtime IDs that should alias to peer '{peer_target}'.") print(" Leave blank to skip a platform. Existing aliases are preserved.") diff --git a/tests/honcho_plugin/test_cli.py b/tests/honcho_plugin/test_cli.py index 74b7e1bc34e..fcbce52703b 100644 --- a/tests/honcho_plugin/test_cli.py +++ b/tests/honcho_plugin/test_cli.py @@ -239,7 +239,7 @@ class TestCloneHonchoForProfile: """Identity-key carryover during profile cloning. The host-scoped identity-mapping keys (``userPeerAliases``, - ``runtimePeerPrefix``, ``pinPeerName``) must survive a clone; otherwise + ``runtimePeerPrefix``, ``pinUserPeer``) must survive a clone; otherwise the new profile silently fragments memory by resolving gateway users to raw runtime IDs instead of operator-declared peers. """ @@ -290,7 +290,7 @@ class TestCloneHonchoForProfile: new_block = written["cfg"]["hosts"]["hermes_coder"] assert new_block["runtimePeerPrefix"] == "telegram_" - def test_pin_peer_name_carries_into_cloned_profile(self, monkeypatch, tmp_path): + def test_legacy_pin_peer_name_migrates_to_canonical_on_clone(self, monkeypatch, tmp_path): cfg = { "apiKey": "***", "hosts": { @@ -304,7 +304,8 @@ class TestCloneHonchoForProfile: ok = honcho_cli.clone_honcho_for_profile("coder") assert ok is True new_block = written["cfg"]["hosts"]["hermes_coder"] - assert new_block["pinPeerName"] is True + assert new_block["pinUserPeer"] is True + assert "pinPeerName" not in new_block def test_unset_identity_keys_do_not_appear_in_cloned_profile(self, monkeypatch, tmp_path): cfg = { @@ -317,6 +318,7 @@ class TestCloneHonchoForProfile: new_block = written["cfg"]["hosts"]["hermes_coder"] assert "userPeerAliases" not in new_block assert "runtimePeerPrefix" not in new_block + assert "pinUserPeer" not in new_block assert "pinPeerName" not in new_block @@ -409,7 +411,7 @@ class TestSetupWizardDeploymentShape: }}, } host = self._run_setup(monkeypatch, tmp_path, answers=answers, initial_cfg=initial_cfg) - assert host["pinPeerName"] is True + assert host["pinUserPeer"] is True assert "userPeerAliases" not in host assert "runtimePeerPrefix" not in host @@ -424,7 +426,7 @@ class TestSetupWizardDeploymentShape: "telegram_", # runtime peer prefix ] host = self._run_setup(monkeypatch, tmp_path, answers=answers) - assert host["pinPeerName"] is False + assert host["pinUserPeer"] is False # 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. @@ -446,7 +448,7 @@ class TestSetupWizardDeploymentShape: "", # runtime peer prefix (skip) ] host = self._run_setup(monkeypatch, tmp_path, answers=answers) - assert host["pinPeerName"] is False + assert host["pinUserPeer"] is False assert host["userPeerAliases"] == { "86701400": "eri", "491827364": "eri", @@ -454,6 +456,8 @@ class TestSetupWizardDeploymentShape: assert "runtimePeerPrefix" not in host def test_skip_shape_preserves_existing_identity_config(self, monkeypatch, tmp_path): + # Seeds the legacy ``pinPeerName``: skip must leave the mapping intact + # except for the on-load migration onto the canonical key. initial_cfg = { "apiKey": "***", "hosts": {"hermes": { @@ -466,7 +470,8 @@ class TestSetupWizardDeploymentShape: "cloud", "", "eri", "hermetika", "hermes", "skip", ] host = self._run_setup(monkeypatch, tmp_path, answers=answers, initial_cfg=initial_cfg) - assert host["pinPeerName"] is True + assert host["pinUserPeer"] is True + assert "pinPeerName" not in host assert host["userPeerAliases"] == {"keep": "me"} assert host["runtimePeerPrefix"] == "keep_" @@ -494,7 +499,7 @@ class TestSetupWizardDeploymentShape: "", # runtime prefix (skip) ] host = self._run_setup(monkeypatch, tmp_path, answers=answers, initial_cfg=initial_cfg) - assert host["pinPeerName"] is False + assert host["pinUserPeer"] is False assert host["userPeerAliases"] == {"86701400": "eri"} def test_single_to_multi_yes_override_keeps_multi(self, monkeypatch, tmp_path): @@ -512,7 +517,7 @@ class TestSetupWizardDeploymentShape: "telegram_", # runtime peer prefix ] host = self._run_setup(monkeypatch, tmp_path, answers=answers, initial_cfg=initial_cfg) - assert host["pinPeerName"] is False + assert host["pinUserPeer"] is False # See test_multi_shape_leaves_pin_false_and_accepts_prefix. assert "userPeerAliases" not in host assert host["runtimePeerPrefix"] == "telegram_" @@ -535,10 +540,9 @@ class TestSetupWizardDeploymentShape: # 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 + # Scrub-then-write normalises onto the canonical pinUserPeer. + assert host["pinUserPeer"] is True + assert "pinPeerName" not in host def test_host_pin_user_peer_false_overrides_root_pin_peer_name( self, monkeypatch, tmp_path @@ -558,8 +562,8 @@ class TestSetupWizardDeploymentShape: } 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 + assert host["pinUserPeer"] is False + assert "pinPeerName" not in host def test_root_user_peer_aliases_detected_as_hybrid(self, monkeypatch, tmp_path): """Root-level ``userPeerAliases`` must classify as ``hybrid`` even @@ -572,7 +576,7 @@ class TestSetupWizardDeploymentShape: } answers = ["cloud", "", "eri", "hermetika", "hermes"] host = self._run_setup(monkeypatch, tmp_path, answers=answers, initial_cfg=initial_cfg) - assert host["pinPeerName"] is False + assert host["pinUserPeer"] 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"} @@ -584,7 +588,7 @@ class TestSetupWizardDeploymentShape: 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 + We honor that by writing ``pinUserPeer: 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. @@ -599,14 +603,12 @@ class TestSetupWizardDeploymentShape: "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 host["pinUserPeer"] 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). + """Choosing ``single`` must overwrite a stale ``pinUserPeer: false`` + with ``pinUserPeer: true`` so the profile ends up genuinely pinned. """ initial_cfg = { "apiKey": "***", @@ -620,8 +622,7 @@ class TestSetupWizardDeploymentShape: "single", ] host = self._run_setup(monkeypatch, tmp_path, answers=answers, initial_cfg=initial_cfg) - assert host["pinPeerName"] is True - assert "pinUserPeer" not in host + assert host["pinUserPeer"] is True class TestCloneCarriesPinUserPeer: @@ -653,3 +654,27 @@ class TestCloneCarriesPinUserPeer: assert ok is True new_block = written["cfg"]["hosts"]["hermes_partner"] assert new_block["pinUserPeer"] is True + + +class TestMigratePinKey: + """``_migrate_pin_key`` rewrites the legacy ``pinPeerName`` onto the + canonical ``pinUserPeer`` in place, without clobbering an existing + canonical value.""" + + def test_legacy_key_renamed_to_canonical(self): + import plugins.memory.honcho.cli as honcho_cli + block = {"pinPeerName": True} + assert honcho_cli._migrate_pin_key(block) is True + assert block == {"pinUserPeer": True} + + def test_canonical_key_wins_when_both_present(self): + import plugins.memory.honcho.cli as honcho_cli + block = {"pinPeerName": True, "pinUserPeer": False} + assert honcho_cli._migrate_pin_key(block) is True + assert block == {"pinUserPeer": False} + + def test_noop_when_no_legacy_key(self): + import plugins.memory.honcho.cli as honcho_cli + block = {"pinUserPeer": True} + assert honcho_cli._migrate_pin_key(block) is False + assert block == {"pinUserPeer": True}