fix(install): never clobber an unparseable settings file; back up before write (#2167)

The hook installers fell back to settings={} on any JSON parse error and
then overwrote the whole file, destroying the user's config (the likely
trigger is a UTF-8 BOM, same class as #2163). All four installers now
read utf-8-sig, refuse to modify a file that isn't a JSON object (naming
the path) instead of clobbering it, back up to <name>.graphify-bak before
any modifying write, skip the write when content is unchanged, and guard
the PreToolUse filter against non-dict entries.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

graphify/install.py

@@ -19,6 +19,7 @@ import re
 import shutil
 import sys
 from pathlib import Path
+from typing import NoReturn
 
 try:
     from importlib.metadata import version as _pkg_version
@@ -718,23 +719,63 @@ def gemini_install(project_dir: Path | None = None, *, project: bool = False) ->
     print()
     print("Gemini CLI will now check the knowledge graph before answering")
     print("codebase questions and rebuild it after code changes.")
+def _refuse_to_modify(settings_path: Path) -> "NoReturn":
+    """Abort a hook install rather than clobber a config file we can't parse (#2167)."""
+    print(
+        f"[graphify] refusing to modify {settings_path}: not valid JSON "
+        "(fix or move it and re-run)",
+        file=sys.stderr,
+    )
+    sys.exit(1)
+def _read_settings_for_merge(settings_path: Path) -> dict:
+    """Load an existing settings/hooks JSON file for a read-modify-write merge.
+
+    A missing file yields a fresh ``{}`` (first install). An existing file that
+    cannot be parsed as a JSON object aborts via ``_refuse_to_modify`` instead of
+    silently falling back to ``{}`` — the old fallback rewrote the whole file and
+    destroyed every setting the user had (#2167). Reads with ``utf-8-sig`` so a
+    UTF-8 BOM (the most likely parse-error trigger, same class as #2163) is
+    tolerated rather than fatal.
+    """
+    if not settings_path.exists():
+        return {}
+    try:
+        settings = json.loads(settings_path.read_text(encoding="utf-8-sig"))
+    except (json.JSONDecodeError, UnicodeDecodeError, OSError):
+        settings = None
+    if not isinstance(settings, dict):
+        _refuse_to_modify(settings_path)
+    return settings
+def _write_settings_with_backup(settings_path: Path, settings: dict) -> None:
+    """Serialize ``settings`` to ``settings_path``, backing up the previous file.
+
+    Skips the write entirely when the output is identical to what is on disk
+    (idempotent re-install: no backup churn, no mtime churn). Otherwise copies
+    the existing file to ``<name>.graphify-bak`` (single rolling backup) before
+    overwriting, so one bad merge can never destroy the user's config (#2167).
+    """
+    output = json.dumps(settings, indent=2)
+    if settings_path.exists():
+        if settings_path.read_text(encoding="utf-8") == output:
+            return
+        backup = settings_path.with_name(settings_path.name + ".graphify-bak")
+        shutil.copy2(settings_path, backup)
+    settings_path.write_text(output, encoding="utf-8")
 def _install_gemini_hook(project_dir: Path) -> None:
     settings_path = project_dir / ".gemini" / "settings.json"
     settings_path.parent.mkdir(parents=True, exist_ok=True)
-    try:
-        settings = (
-            json.loads(settings_path.read_text(encoding="utf-8"))
-            if settings_path.exists()
-            else {}
-        )
-    except json.JSONDecodeError:
-        settings = {}
-    before_tool = settings.setdefault("hooks", {}).setdefault("BeforeTool", [])
-    settings["hooks"]["BeforeTool"] = [
+    settings = _read_settings_for_merge(settings_path)
+    hooks = settings.setdefault("hooks", {})
+    if not isinstance(hooks, dict):
+        _refuse_to_modify(settings_path)
+    before_tool = hooks.setdefault("BeforeTool", [])
+    if not isinstance(before_tool, list):
+        _refuse_to_modify(settings_path)
+    hooks["BeforeTool"] = [
         h for h in before_tool if "graphify" not in str(h)
     ]
-    settings["hooks"]["BeforeTool"].append(_gemini_hook())
-    settings_path.write_text(json.dumps(settings, indent=2), encoding="utf-8")
+    hooks["BeforeTool"].append(_gemini_hook())
+    _write_settings_with_backup(settings_path, settings)
     print("  .gemini/settings.json  ->  BeforeTool hook registered")
 def _uninstall_gemini_hook(project_dir: Path) -> None:
     settings_path = project_dir / ".gemini" / "settings.json"
@@ -1364,13 +1405,7 @@ def _install_codex_hook(project_dir: Path) -> None:
     hooks_path = project_dir / ".codex" / "hooks.json"
     hooks_path.parent.mkdir(parents=True, exist_ok=True)
 
-    if hooks_path.exists():
-        try:
-            existing = json.loads(hooks_path.read_text(encoding="utf-8"))
-        except json.JSONDecodeError:
-            existing = {}
-    else:
-        existing = {}
+    existing = _read_settings_for_merge(hooks_path)
 
     graphify_exe = _resolve_graphify_exe()
     hook_entry = {
@@ -1384,10 +1419,15 @@ def _install_codex_hook(project_dir: Path) -> None:
         }
     }
 
-    pre_tool = existing.setdefault("hooks", {}).setdefault("PreToolUse", [])
-    existing["hooks"]["PreToolUse"] = [h for h in pre_tool if "graphify" not in str(h)]
-    existing["hooks"]["PreToolUse"].extend(hook_entry["hooks"]["PreToolUse"])
-    hooks_path.write_text(json.dumps(existing, indent=2), encoding="utf-8")
+    hooks = existing.setdefault("hooks", {})
+    if not isinstance(hooks, dict):
+        _refuse_to_modify(hooks_path)
+    pre_tool = hooks.setdefault("PreToolUse", [])
+    if not isinstance(pre_tool, list):
+        _refuse_to_modify(hooks_path)
+    hooks["PreToolUse"] = [h for h in pre_tool if "graphify" not in str(h)]
+    hooks["PreToolUse"].extend(hook_entry["hooks"]["PreToolUse"])
+    _write_settings_with_backup(hooks_path, existing)
     print(f"  .codex/hooks.json  ->  PreToolUse hook registered ({graphify_exe} hook-check)")
 def _uninstall_codex_hook(project_dir: Path) -> None:
     """Remove graphify PreToolUse hook from .codex/hooks.json."""
@@ -1670,20 +1710,18 @@ def _install_claude_hook(project_dir: Path, strict: bool = False) -> None:
     settings_path = project_dir / ".claude" / "settings.json"
     settings_path.parent.mkdir(parents=True, exist_ok=True)
 
-    if settings_path.exists():
-        try:
-            settings = json.loads(settings_path.read_text(encoding="utf-8"))
-        except json.JSONDecodeError:
-            settings = {}
-    else:
-        settings = {}
+    settings = _read_settings_for_merge(settings_path)
 
     hooks = settings.setdefault("hooks", {})
+    if not isinstance(hooks, dict):
+        _refuse_to_modify(settings_path)
     pre_tool = hooks.setdefault("PreToolUse", [])
+    if not isinstance(pre_tool, list):
+        _refuse_to_modify(settings_path)
 
-    hooks["PreToolUse"] = [h for h in pre_tool if not (h.get("matcher") in ("Glob|Grep", "Bash", "Bash|Grep", "Read|Glob") and "graphify" in str(h))]
+    hooks["PreToolUse"] = [h for h in pre_tool if not (isinstance(h, dict) and h.get("matcher") in ("Glob|Grep", "Bash", "Bash|Grep", "Read|Glob") and "graphify" in str(h))]
     hooks["PreToolUse"].extend(_claude_pretooluse_hooks(strict=strict))
-    settings_path.write_text(json.dumps(settings, indent=2), encoding="utf-8")
+    _write_settings_with_backup(settings_path, settings)
     _mode = " (strict)" if strict else ""
     print(f"  .claude/settings.json  ->  PreToolUse hooks registered (Bash|Grep search + Read/Glob){_mode}")
 def _uninstall_claude_hook(project_dir: Path) -> None:
@@ -1841,20 +1879,18 @@ def _install_codebuddy_hook(project_dir: Path) -> None:
     settings_path = project_dir / ".codebuddy" / "settings.json"
     settings_path.parent.mkdir(parents=True, exist_ok=True)
 
-    if settings_path.exists():
-        try:
-            settings = json.loads(settings_path.read_text(encoding="utf-8"))
-        except json.JSONDecodeError:
-            settings = {}
-    else:
-        settings = {}
+    settings = _read_settings_for_merge(settings_path)
 
     hooks = settings.setdefault("hooks", {})
+    if not isinstance(hooks, dict):
+        _refuse_to_modify(settings_path)
     pre_tool = hooks.setdefault("PreToolUse", [])
+    if not isinstance(pre_tool, list):
+        _refuse_to_modify(settings_path)
 
-    hooks["PreToolUse"] = [h for h in pre_tool if not (h.get("matcher") in ("Glob|Grep", "Bash", "Bash|Grep", "Read|Glob") and "graphify" in str(h))]
+    hooks["PreToolUse"] = [h for h in pre_tool if not (isinstance(h, dict) and h.get("matcher") in ("Glob|Grep", "Bash", "Bash|Grep", "Read|Glob") and "graphify" in str(h))]
     hooks["PreToolUse"].extend(_claude_pretooluse_hooks())
-    settings_path.write_text(json.dumps(settings, indent=2), encoding="utf-8")
+    _write_settings_with_backup(settings_path, settings)
     print(f"  .codebuddy/settings.json  ->  PreToolUse hooks registered")
 def _uninstall_codebuddy_hook(project_dir: Path) -> None:
     """Remove graphify PreToolUse hook from .codebuddy/settings.json."""

tests/test_settings_merge.py

@@ -0,0 +1,204 @@
+"""Regression tests for issue #2167: hook installers must merge into existing
+settings/hooks JSON files, never clobber them.
+
+The old behavior fell back to ``settings = {}`` on any parse error (a UTF-8 BOM
+was enough, same class as #2163) and then rewrote the whole file, destroying the
+user's mcpServers/enabledPlugins/theme/hooks. The fix:
+
+- read with utf-8-sig (BOM-tolerant),
+- refuse to touch an existing file that is not a JSON object (stderr + exit 1),
+- back up to <name>.graphify-bak before any modifying write,
+- skip the write entirely when nothing changed (idempotent re-install),
+- never crash on (and always preserve) non-dict hook entries.
+"""
+
+from __future__ import annotations
+
+import json
+from pathlib import Path
+
+import pytest
+
+from graphify.install import (
+    _install_claude_hook,
+    _install_codebuddy_hook,
+    _install_codex_hook,
+    _install_gemini_hook,
+)
+
+# installer key -> (function, settings file relative to project dir, hooks section)
+_INSTALLERS = {
+    "claude": (_install_claude_hook, Path(".claude") / "settings.json", "PreToolUse"),
+    "codebuddy": (_install_codebuddy_hook, Path(".codebuddy") / "settings.json", "PreToolUse"),
+    "codex": (_install_codex_hook, Path(".codex") / "hooks.json", "PreToolUse"),
+    "gemini": (_install_gemini_hook, Path(".gemini") / "settings.json", "BeforeTool"),
+}
+
+ALL_INSTALLERS = pytest.mark.parametrize("installer", sorted(_INSTALLERS), ids=sorted(_INSTALLERS))
+
+
+def _seed(tmp_path: Path, installer: str, payload) -> Path:
+    _, rel, _ = _INSTALLERS[installer]
+    settings_path = tmp_path / rel
+    settings_path.parent.mkdir(parents=True, exist_ok=True)
+    if isinstance(payload, bytes):
+        settings_path.write_bytes(payload)
+    else:
+        settings_path.write_text(json.dumps(payload, indent=2), encoding="utf-8")
+    return settings_path
+
+
+def _run(tmp_path: Path, installer: str, **kwargs) -> Path:
+    fn, rel, _ = _INSTALLERS[installer]
+    fn(tmp_path, **kwargs)
+    return tmp_path / rel
+
+
+# ---------------------------------------------------------------- merge
+
+
+def test_claude_install_preserves_existing_settings(tmp_path):
+    """#2167 core case: every key graphify does not own must survive install."""
+    seeded = {
+        "mcpServers": {"context7": {"command": "npx", "args": ["context7"]}},
+        "enabledPlugins": ["my-plugin@marketplace"],
+        "theme": "dark",
+        "hooks": {
+            "PostToolUse": [
+                {"matcher": "Bash", "hooks": [{"type": "command", "command": "my-formatter"}]}
+            ],
+            "PreToolUse": [
+                {"matcher": "Write", "hooks": [{"type": "command", "command": "my-write-guard"}]}
+            ],
+        },
+    }
+    settings_path = _seed(tmp_path, "claude", seeded)
+
+    _run(tmp_path, "claude", strict=True)
+
+    result = json.loads(settings_path.read_text(encoding="utf-8"))
+    # top-level keys graphify does not own are untouched
+    assert result["mcpServers"] == seeded["mcpServers"]
+    assert result["enabledPlugins"] == seeded["enabledPlugins"]
+    assert result["theme"] == "dark"
+    # hooks sections graphify does not manage are untouched
+    assert result["hooks"]["PostToolUse"] == seeded["hooks"]["PostToolUse"]
+    # the user's own PreToolUse entry survives alongside graphify's
+    pre_tool = result["hooks"]["PreToolUse"]
+    assert seeded["hooks"]["PreToolUse"][0] in pre_tool
+    graphify_hooks = [h for h in pre_tool if "graphify" in str(h)]
+    assert len(graphify_hooks) == 2
+    # strict=True lands on the read guard
+    assert any(h["hooks"][0]["command"].endswith("--strict") for h in graphify_hooks)
+
+
+# ---------------------------------------------------------------- BOM
+
+
+@ALL_INSTALLERS
+def test_bom_settings_are_merged_not_clobbered(tmp_path, installer):
+    """A UTF-8 BOM must not trigger the parse-error path that used to clobber."""
+    seeded = {"mcpServers": {"keep": {"command": "keep-me"}}, "theme": "dark"}
+    body = json.dumps(seeded, indent=2).encode("utf-8")
+    settings_path = _seed(tmp_path, installer, b"\xef\xbb\xbf" + body)
+
+    _run(tmp_path, installer)
+
+    result = json.loads(settings_path.read_text(encoding="utf-8"))
+    assert result["mcpServers"] == seeded["mcpServers"]
+    assert result["theme"] == "dark"
+    section = _INSTALLERS[installer][2]
+    assert any("graphify" in str(h) for h in result["hooks"][section])
+
+
+# ---------------------------------------------------------------- invalid JSON
+
+
+@ALL_INSTALLERS
+def test_invalid_json_aborts_without_clobbering(tmp_path, installer, capsys):
+    """An unparseable existing file must abort the install, byte-identical on disk."""
+    settings_path = _seed(tmp_path, installer, b"{ not json")
+    original = settings_path.read_bytes()
+
+    with pytest.raises(SystemExit) as excinfo:
+        _run(tmp_path, installer)
+
+    assert excinfo.value.code == 1
+    assert str(settings_path) in capsys.readouterr().err
+    assert settings_path.read_bytes() == original
+    assert not settings_path.with_name(settings_path.name + ".graphify-bak").exists()
+
+
+@ALL_INSTALLERS
+def test_non_object_top_level_aborts_without_clobbering(tmp_path, installer, capsys):
+    """Valid JSON that is not an object (e.g. a list) must also refuse, not crash."""
+    settings_path = _seed(tmp_path, installer, b'["not", "an", "object"]')
+    original = settings_path.read_bytes()
+
+    with pytest.raises(SystemExit) as excinfo:
+        _run(tmp_path, installer)
+
+    assert excinfo.value.code == 1
+    assert str(settings_path) in capsys.readouterr().err
+    assert settings_path.read_bytes() == original
+
+
+def test_non_dict_hooks_section_aborts(tmp_path, capsys):
+    """A malformed hooks value (not a dict) refuses instead of raising/clobbering."""
+    settings_path = _seed(tmp_path, "claude", {"hooks": "oops", "theme": "dark"})
+    original = settings_path.read_bytes()
+
+    with pytest.raises(SystemExit) as excinfo:
+        _run(tmp_path, "claude")
+
+    assert excinfo.value.code == 1
+    assert str(settings_path) in capsys.readouterr().err
+    assert settings_path.read_bytes() == original
+
+
+# ---------------------------------------------------------------- backup
+
+
+@ALL_INSTALLERS
+def test_backup_written_before_modify_and_stable_on_reinstall(tmp_path, installer):
+    seeded = {"theme": "dark", "mcpServers": {"keep": {}}}
+    settings_path = _seed(tmp_path, installer, seeded)
+    pre_write = settings_path.read_text(encoding="utf-8")
+    backup = settings_path.with_name(settings_path.name + ".graphify-bak")
+
+    _run(tmp_path, installer)
+
+    assert backup.exists()
+    assert backup.read_text(encoding="utf-8") == pre_write
+    merged = settings_path.read_text(encoding="utf-8")
+    assert merged != pre_write  # sanity: the run was a modifying one
+
+    # Idempotent second run: output is unchanged, so neither the settings file
+    # nor the backup may be rewritten (the backup keeps the pre-graphify content).
+    _run(tmp_path, installer)
+    assert settings_path.read_text(encoding="utf-8") == merged
+    assert backup.read_text(encoding="utf-8") == pre_write
+
+
+def test_no_backup_on_fresh_install(tmp_path):
+    settings_path = _run(tmp_path, "claude")
+    assert settings_path.exists()
+    assert not settings_path.with_name(settings_path.name + ".graphify-bak").exists()
+
+
+# ---------------------------------------------------------------- non-dict entries
+
+
+@ALL_INSTALLERS
+def test_non_dict_hook_entry_is_preserved_not_fatal(tmp_path, installer):
+    """A legacy non-dict entry in the managed section must not crash the filter
+    (the old claude/codebuddy filter called h.get() unconditionally) and must
+    survive the merge."""
+    section = _INSTALLERS[installer][2]
+    settings_path = _seed(tmp_path, installer, {"hooks": {section: ["legacy-string"]}})
+
+    _run(tmp_path, installer)
+
+    entries = json.loads(settings_path.read_text(encoding="utf-8"))["hooks"][section]
+    assert "legacy-string" in entries
+    assert any("graphify" in str(h) for h in entries)