From bb06e1621ed25c588fa2dab8a6dd26a0fb7d42d3 Mon Sep 17 00:00:00 2001 From: Erosika Date: Fri, 4 Sep 2026 15:30:19 -0400 Subject: [PATCH] fix(honcho): refuse to rewrite a honcho.json that exists but does not parse a truncated or hand-edited honcho.json reads as {} on the tolerant read path. the next write then replaced the file with only the current host block: an oauth refresh, a login, or any 'hermes honcho' command that saves a setting wiped every other host and the root keys. _read_config_strict now raises on a parse error the same way it raises on a read error, and logs one sentence naming the file. _rotate_and_persist treats that as a refresh failure and enters the cooldown without spending the refresh token. install_grant raises into the setup flow. in cli.py every write goes through _write_config, which runs the same check first and raises ConfigWriteRefused; the honcho router, the setup wizard and the profile sync print the sentence instead of a traceback. the wizard checks before asking its questions. read paths keep the {} fallback. follows #95860, which added the strict reader for unreadable files and kept a .corrupt copy on a parse error. leaving the original file in place keeps the same bytes without a second copy of the tokens on disk. --- plugins/memory/honcho/cli.py | 35 ++++++++++++++++++++-- plugins/memory/honcho/oauth.py | 30 ++++--------------- tests/honcho_plugin/test_cli.py | 49 +++++++++++++++++++++++++++++++ tests/honcho_plugin/test_oauth.py | 36 +++++++++++++++++++---- 4 files changed, 118 insertions(+), 32 deletions(-) diff --git a/plugins/memory/honcho/cli.py b/plugins/memory/honcho/cli.py index 722390f83e..fb9dcd7868 100644 --- a/plugins/memory/honcho/cli.py +++ b/plugins/memory/honcho/cli.py @@ -70,8 +70,24 @@ def _read_config() -> dict: return read_json_or_empty(_config_path()) +class ConfigWriteRefused(Exception): + """honcho.json exists on disk but does not parse, so no command may overwrite it.""" + + +def _refuse_unparseable(path: Path) -> None: + """Raise ``ConfigWriteRefused`` when ``path`` exists but cannot be read or parsed. ``_read_config`` + returns ``{}`` for such a file, and writing that back would drop every other host and root key.""" + from plugins.memory.honcho.oauth import _read_config_strict + try: + _read_config_strict(path) + except (OSError, ValueError) as e: + raise ConfigWriteRefused(f"{path} exists but could not be read as JSON ({e}). Nothing was written. " + "Fix or move the file, then re-run.") from e + + def _write_config(cfg: dict, path: Path | None = None) -> None: path = path or _local_config_path() + _refuse_unparseable(path) path.parent.mkdir(parents=True, exist_ok=True) from utils import atomic_json_write atomic_json_write(path, cfg, mode=0o600) @@ -240,7 +256,11 @@ def _sync_profiles(verbose: bool) -> int: created = skipped = 0 for p in (p for p in profiles if p.name != "default"): - if clone_honcho_for_profile(p.name): + try: + cloned = clone_honcho_for_profile(p.name) + except ConfigWriteRefused as e: + return say(f" {e}\n") or created + if cloned: say(f" + {p.name} -> {profile_host_key(p.name)}") created += 1 else: @@ -702,8 +722,16 @@ def _setup_tuning(cfg: dict, hermes_host: dict) -> None: def cmd_setup(args) -> None: """Interactive Honcho setup wizard.""" + try: + _setup_wizard(args) + except ConfigWriteRefused as e: + print(f" {e}\n") + + +def _setup_wizard(args) -> None: cfg = _read_config() write_path, read_path = _local_config_path(), _config_path() + _refuse_unparseable(write_path) # before the questions, not after them print(f"\nHoncho memory setup\n{RULE}\n Honcho gives Hermes persistent cross-session memory.\n Config: {write_path}") if read_path != write_path and read_path.exists(): print(f" (seeding from existing config at {read_path})") @@ -1341,7 +1369,10 @@ def honcho_command(args) -> None: if handler is None: return print(f" Unknown honcho command: {sub}\n" " Available: status, sessions, map, peer, mode, strategy, tokens, identity, migrate, enable, disable, sync\n") - handler(args) + try: + handler(args) + except ConfigWriteRefused as e: + print(f" {e}\n") def register_cli(subparser) -> None: diff --git a/plugins/memory/honcho/oauth.py b/plugins/memory/honcho/oauth.py index 94e87429c9..3b90b9880c 100644 --- a/plugins/memory/honcho/oauth.py +++ b/plugins/memory/honcho/oauth.py @@ -108,34 +108,16 @@ def _mark_grant_dead(key: tuple[str, str], cred: OAuthCredential) -> None: _reauth_check_cache.pop(key, None) # verdict changed without a config rewrite def _read_config_strict(path: Path) -> dict[str, Any]: - """Reader for the WRITE paths: never lets a failed read become an empty store. - - A missing file is the bootstrap case and reads as ``{}``. A file that EXISTS but cannot be read - (EACCES after a root-owned write, EIO, a stalled mount) raises instead: ``atomic_json_write`` replaces - the whole file and ``os.replace`` needs only a writable parent, so degrading here would let the next - persist erase every other host's credentials. Genuine corruption still degrades, but only after - preserving a ``.corrupt`` copy: a truncated store usually holds the other hosts' tokens verbatim. - """ + """Reader for the WRITE paths. A missing file reads as ``{}``. A file that exists but cannot be read + or parsed raises so the caller leaves it alone: ``atomic_json_write`` replaces the whole file, and a + write seeded from ``{}`` would erase every other host's credentials and the root keys.""" try: return json.loads(path.read_text(encoding="utf-8-sig")) except FileNotFoundError: return {} - except OSError: - logger.warning("Honcho config at %s could not be read; refusing to treat it as empty " - "(a persist would erase every other host's credentials)", path, exc_info=True) + except (OSError, ValueError) as exc: + logger.warning("Honcho config at %s exists but could not be read (%s); refusing to overwrite it.", path, exc) raise - except json.JSONDecodeError as exc: - corrupt = path.with_name(path.name + ".corrupt") - preserved = False - try: - import shutil - shutil.copy2(path, corrupt) - preserved = True - except Exception: - logger.debug("could not preserve a copy at %s", corrupt, exc_info=True) - logger.warning("Honcho config at %s is corrupt (%s); starting from an empty store. %s", path, exc, - f"Original preserved at {corrupt}" if preserved else f"A copy could NOT be preserved at {corrupt}") - return {} def _load_cred(path: Path, host: str, raw: dict[str, Any] | None = None) -> OAuthCredential | None: """Credential from ``host``'s block in ``raw`` (or the file at ``path``).""" @@ -285,7 +267,7 @@ def _rotate_and_persist( try: # Read before spending the single-use refresh token: an exchange that cannot be persisted loses the grant. raw = _read_config_strict(path) - except OSError: + except (OSError, ValueError): _refresh_failure_at[key] = time.monotonic() return None try: diff --git a/tests/honcho_plugin/test_cli.py b/tests/honcho_plugin/test_cli.py index fd9cfbb154..3b3528a0cd 100644 --- a/tests/honcho_plugin/test_cli.py +++ b/tests/honcho_plugin/test_cli.py @@ -3,6 +3,8 @@ from types import SimpleNamespace import json +import pytest + class TestResolveApiKey: """Test _resolve_api_key with various config shapes.""" @@ -744,3 +746,50 @@ class TestCmdSetupDeviceFlow: assert len(calls) == 1 assert "apiKey" not in cfg.get("hosts", {}).get("hermes", {}) + + +class TestWriteRefusesUnparseableStore: + """A honcho.json that exists but does not parse reads as {} on the tolerant path. Writing that + back would replace every host and root key with the current command's block, so writes refuse.""" + + def _point_at(self, monkeypatch, cfg_path): + import plugins.memory.honcho.cli as honcho_cli + monkeypatch.setattr(honcho_cli, "_config_path", lambda: cfg_path) + monkeypatch.setattr(honcho_cli, "_local_config_path", lambda: cfg_path) + monkeypatch.setattr(honcho_cli, "_host_key", lambda: "hermes_coder") + monkeypatch.setattr(honcho_cli, "_ensure_peer_exists", lambda host_key=None: False) + return honcho_cli + + def test_write_config_refuses_and_leaves_file_alone(self, monkeypatch, tmp_path): + cfg_path = tmp_path / "honcho.json" + cfg_path.write_text('{"hosts": {"hermes": {"apiKey": "k"', encoding="utf-8") + honcho_cli = self._point_at(monkeypatch, cfg_path) + with pytest.raises(honcho_cli.ConfigWriteRefused): + honcho_cli._write_config({"hosts": {"hermes_coder": {"enabled": True}}}) + assert cfg_path.read_text(encoding="utf-8") == '{"hosts": {"hermes": {"apiKey": "k"' + + def test_write_config_still_bootstraps_a_missing_file(self, monkeypatch, tmp_path): + cfg_path = tmp_path / "honcho.json" + honcho_cli = self._point_at(monkeypatch, cfg_path) + honcho_cli._write_config({"apiKey": "k"}) + assert json.loads(cfg_path.read_text(encoding="utf-8")) == {"apiKey": "k"} + + def test_enable_prints_one_sentence_and_writes_nothing(self, monkeypatch, tmp_path, capsys): + cfg_path = tmp_path / "honcho.json" + cfg_path.write_text("{not json", encoding="utf-8") + honcho_cli = self._point_at(monkeypatch, cfg_path) + monkeypatch.setenv("HONCHO_API_KEY", "env-key") + honcho_cli.honcho_command(SimpleNamespace(honcho_command="enable", target_profile=None)) + out = capsys.readouterr().out + assert "could not be read as JSON" in out + assert "Nothing was written" in out + assert cfg_path.read_text(encoding="utf-8") == "{not json" + + def test_setup_refuses_before_asking_anything(self, monkeypatch, tmp_path, capsys): + cfg_path = tmp_path / "honcho.json" + cfg_path.write_text("{not json", encoding="utf-8") + honcho_cli = self._point_at(monkeypatch, cfg_path) + monkeypatch.setattr(honcho_cli, "_prompt", lambda *a, **k: pytest.fail("wizard asked a question")) + honcho_cli.cmd_setup(SimpleNamespace()) + assert "Nothing was written" in capsys.readouterr().out + assert cfg_path.read_text(encoding="utf-8") == "{not json" diff --git a/tests/honcho_plugin/test_oauth.py b/tests/honcho_plugin/test_oauth.py index a0cbb55bf5..e3116111f2 100644 --- a/tests/honcho_plugin/test_oauth.py +++ b/tests/honcho_plugin/test_oauth.py @@ -272,17 +272,42 @@ class TestPersistReadFailure: oauth._refresh_failure_at.pop(key, None) assert path.read_bytes() == before - def test_corrupt_store_degrades_but_preserves_a_copy(self, tmp_path): + def test_corrupt_store_refuses_to_persist(self, tmp_path): path = tmp_path / "honcho.json" truncated = json.dumps( {"hosts": {"keep.example": _host_block(refresh="hch-rt-keep")}} )[:-5] path.write_text(truncated, encoding="utf-8") + cred = OAuthCredential.from_host_block(_host_block(refresh="hch-rt-new")) - assert oauth._read_config_strict(path) == {} - corrupt = path.with_name(path.name + ".corrupt") - assert corrupt.exists() - assert corrupt.read_text(encoding="utf-8") == truncated + with pytest.raises(ValueError): + oauth._read_config_strict(path) + with pytest.raises(ValueError): + oauth._persist_credential(path, "keep.example", cred) + with pytest.raises(ValueError): + oauth.install_grant( + path, "new.example", + {"access_token": "hch-at-new", "refresh_token": "hch-rt-new", "expires_in": 3600}, + client_id="hermes-desktop", token_endpoint="http://localhost:8000/oauth/token", + ) + assert path.read_text(encoding="utf-8") == truncated + assert not path.with_name(path.name + ".corrupt").exists() + + def test_corrupt_store_fails_the_refresh_before_the_exchange(self, tmp_path, monkeypatch): + path = tmp_path / "honcho.json" + path.write_text("{not json", encoding="utf-8") + cred = OAuthCredential.from_host_block(_host_block(refresh="hch-rt-rot")) + key = (str(path), "rotate.example") + oauth._refresh_failure_at.pop(key, None) + + def no_exchange(*a, **kw): # pragma: no cover - must not run + raise AssertionError("exchange ran against a corrupt store") + + monkeypatch.setattr(oauth, "_exchange_with_retry", no_exchange) + assert oauth._rotate_and_persist(path, "rotate.example", key, cred, now=1.0) is None + assert key in oauth._refresh_failure_at + oauth._refresh_failure_at.pop(key, None) + assert path.read_text(encoding="utf-8") == "{not json" def test_missing_store_still_bootstraps_empty(self, tmp_path): assert oauth._read_config_strict(tmp_path / "honcho.json") == {} @@ -295,7 +320,6 @@ class TestPersistReadFailure: ) assert "keep.example" in oauth._read_config(path).get("hosts", {}) assert "keep.example" in oauth._read_config_strict(path).get("hosts", {}) - assert not path.with_name(path.name + ".corrupt").exists() def test_install_grant_raises_and_preserves_store_when_unreadable( self, tmp_path, monkeypatch