fix: hook graceful degradation + install safety (#312, #344, #350)

code_review_graph/cli.py

@@ -225,9 +225,26 @@ def _handle_init(args: argparse.Namespace) -> None:
         install_qoder_skills,
     )
 
-    if not skip_skills:
+    # #350: ``generate_skills`` writes Claude Code skill markdown to
+    # ``<repo>/.claude/skills/``.  That directory is only read by Claude
+    # Code (and by Qoder via a separate global install path).  Previously
+    # we generated it for every platform, which confused Cursor / Windsurf /
+    # Zed users into thinking the tool created Claude config against their
+    # will.  Restrict skill generation to Claude-family targets only; other
+    # platforms get the relevant ``<platform>rules`` / ``AGENTS.md``
+    # injection instead (handled below).
+    _claude_skill_targets = {"claude", "claude-code", "qoder", "all"}
+    if not skip_skills and target in _claude_skill_targets:
         skills_dir = generate_skills(repo_root)
         print(f"Generated skills in {skills_dir}")
+    elif not skip_skills:
+        # Non-Claude target: make the decision explicit so users know the
+        # platform-appropriate instructions live elsewhere.
+        print(
+            f"Skipped generating .claude/skills/ for platform '{target}' — "
+            f"skills are read only by Claude Code / Qoder.  Use "
+            f"--platform all (or --platform claude) to generate them."
+        )
 
     # Confirm before writing instruction files (#173). --yes skips the
     # prompt; --no-instructions skips the whole block.
@@ -663,12 +680,23 @@ def main() -> None:
         # update and detect-changes require git for diffing
         repo_root = Path(args.repo) if args.repo else find_repo_root()
         if not repo_root:
-            logging.error(
-                "Not in a git repository. '%s' requires git for diffing.",
-                args.command,
+            # #312: Claude Code hooks invoke ``update`` after every
+            # Write/Edit/Bash.  In monorepos with no root ``.git`` (e.g.
+            # frontend/ and backend/ each have their own repo but the
+            # workspace root does not), every tool-use previously spammed
+            # ``PostToolUse:Edit hook error`` because we exited with code
+            # 1.  Emit a friendly warning to stderr (so MCP stdio is not
+            # corrupted) and exit 0 so the hook is non-blocking.  Using
+            # ``print`` instead of ``logging.warning`` because hook/CLI
+            # invocations do not configure the root logger — we want the
+            # message visible without forcing logging setup.
+            print(
+                f"note: not in a git repository; '{args.command}' "
+                f"requires git for diffing, skipping.  Use 'build' for "
+                f"a full parse, or 'git init' to enable incremental updates.",
+                file=sys.stderr,
             )
-            logging.error("Use 'build' for a full parse, or run 'git init' first.")
-            sys.exit(1)
+            sys.exit(0)
     else:
         repo_root = Path(args.repo) if args.repo else find_project_root()
 

code_review_graph/skills.py

@@ -308,14 +308,56 @@ def install_platform_configs(
             configured.append(plat["name"])
             continue
 
-        # Read existing config
+        # Read existing config.
+        # #344: Previously, a JSONDecodeError silently reset ``existing`` to
+        # an empty dict, and the subsequent write clobbered the user's
+        # entire config file (losing Zed themes, keymaps, language servers,
+        # etc.).  Now we refuse to overwrite a malformed config and surface
+        # a clear, actionable error — the user decides whether to fix the
+        # file manually or delete it and retry.
         existing: dict[str, Any] = {}
         if config_path.exists():
             try:
-                existing = json.loads(config_path.read_text(encoding="utf-8", errors="replace"))
-            except (json.JSONDecodeError, OSError):
-                logger.warning("Invalid JSON in %s, will overwrite.", config_path)
-                existing = {}
+                raw = config_path.read_text(encoding="utf-8", errors="replace")
+                existing = json.loads(raw) if raw.strip() else {}
+            except json.JSONDecodeError as exc:
+                msg = (
+                    f"  {plat['name']}: REFUSING to overwrite {config_path} — "
+                    f"the file contains invalid JSON ({exc.msg} at "
+                    f"line {exc.lineno}, col {exc.colno}).  Please fix or "
+                    f"remove this file by hand, then re-run install.  "
+                    f"No changes made."
+                )
+                print(msg)
+                logger.error(
+                    "Invalid JSON in %s (line %d, col %d): %s — refusing "
+                    "to overwrite so user data is not lost.",
+                    config_path, exc.lineno, exc.colno, exc.msg,
+                )
+                continue
+            except OSError as exc:
+                msg = (
+                    f"  {plat['name']}: cannot read {config_path} "
+                    f"({exc.strerror or exc!r}); skipping."
+                )
+                print(msg)
+                logger.error("Cannot read %s: %s", config_path, exc)
+                continue
+        if not isinstance(existing, dict):
+            # File parsed as valid JSON but at the top level is a list or
+            # scalar.  Same data-loss risk — refuse to overwrite.
+            msg = (
+                f"  {plat['name']}: REFUSING to overwrite {config_path} — "
+                f"expected a JSON object at the top level but got "
+                f"{type(existing).__name__}.  Please fix the file by hand "
+                f"or remove it, then re-run install."
+            )
+            print(msg)
+            logger.error(
+                "Unexpected JSON shape in %s: top-level is %s, not object.",
+                config_path, type(existing).__name__,
+            )
+            continue
 
         if plat["format"] == "array":
             arr = existing.get(server_key, [])

tests/test_cli.py

@@ -1,6 +1,7 @@
 """Tests for CLI helpers."""
 
 import logging
+import sys
 from importlib.metadata import PackageNotFoundError
 
 from code_review_graph import cli
@@ -17,3 +18,62 @@ def _raise_package_not_found(_dist_name: str) -> str:
 
     assert version == "dev"
     assert "Package metadata unavailable" in caplog.text
+
+
+class TestUpdateNoGitExitsZero:
+    """Regression tests for #312: running ``update`` or ``detect-changes``
+    in a directory with no git repository must exit 0 (with a warning
+    to stderr) so Claude Code's PostToolUse hook does not report a
+    failure on every Write / Edit / Bash tool call in monorepos where
+    the workspace root has no ``.git``.
+
+    We mock ``find_repo_root`` to return ``None`` explicitly so these
+    tests do not depend on the test runner's ambient git hierarchy
+    (e.g. a ``.git`` directory in the user's home, which would make the
+    unbounded ancestor walk find it and skip the no-git branch we want
+    to test — same hazard addressed by #241's ``stop_at`` parameter).
+    """
+
+    def _invoke(self, command: str, capsys, monkeypatch):
+        """Drive ``cli.main`` through the no-git branch by forcing
+        ``find_repo_root`` to return ``None``, and capture the stderr
+        warning + exit code."""
+        import pytest as _pytest
+
+        monkeypatch.setattr(
+            "code_review_graph.incremental.find_repo_root",
+            lambda *a, **kw: None,
+        )
+        monkeypatch.setattr(sys, "argv", ["code-review-graph", command])
+        with _pytest.raises(SystemExit) as excinfo:
+            cli.main()
+        captured = capsys.readouterr()
+        return excinfo.value.code, captured.out, captured.err
+
+    def test_update_exits_zero_without_git(self, capsys, monkeypatch):
+        """Before #312 this exited 1, causing
+        ``PostToolUse:Edit hook error`` noise on every tool call."""
+        code, _out, err = self._invoke("update", capsys, monkeypatch)
+        assert code == 0, f"expected exit 0, got {code}; stderr: {err!r}"
+
+    def test_update_still_warns_about_missing_git(self, capsys, monkeypatch):
+        """Exit 0 must not be silent — an interactive user still gets
+        told why the update did nothing.  The warning goes to stderr so
+        MCP stdio transport is not corrupted."""
+        _code, out, err = self._invoke("update", capsys, monkeypatch)
+        # Warning must be visible in stderr (hook/MCP-safe location).
+        assert "git" in err.lower(), (
+            f"expected a 'git' hint in stderr; got stdout={out!r} stderr={err!r}"
+        )
+        # And stdout must NOT contain the warning (would corrupt MCP stdio).
+        assert "git" not in out.lower() or "not in a git" not in out.lower()
+
+    def test_detect_changes_also_exits_zero_without_git(
+        self, capsys, monkeypatch,
+    ):
+        """Same non-failing semantics for the sibling ``detect-changes``
+        subcommand — otherwise hooks that wrap it get the same error."""
+        code, _out, err = self._invoke("detect-changes", capsys, monkeypatch)
+        assert code == 0, (
+            f"expected exit 0, got {code}; stderr: {err!r}"
+        )

tests/test_skills.py

@@ -661,6 +661,154 @@ def test_install_qoder_config(self, tmp_path):
         assert data["mcpServers"]["code-review-graph"]["command"] == expected_cmd
 
 
+class TestInstallRefusesToClobberMalformedConfig:
+    """Regression tests for #344: ``install_platform_configs`` previously
+    read existing config files and, on ``JSONDecodeError``, silently
+    reset the in-memory data to ``{}`` and overwrote the file — erasing
+    the user's entire config (Zed themes, keymaps, language servers,
+    etc.).  The fix refuses to overwrite and surfaces a clear error.
+    """
+
+    def _fake_zed_platform(self, settings_path: Path):
+        return patch.dict(
+            PLATFORMS,
+            {
+                "zed": {
+                    **PLATFORMS["zed"],
+                    "config_path": lambda root: settings_path,
+                    "detect": lambda: True,
+                },
+            },
+        )
+
+    def test_zed_refuses_to_overwrite_malformed_json(self, tmp_path, capsys):
+        """The bug: before #344, ``install --platform zed`` would eat a
+        user's entire ``settings.json`` on any JSON parse error."""
+        settings = tmp_path / "zed" / "settings.json"
+        settings.parent.mkdir(parents=True)
+        original = '{ this is not valid JSON }\n'
+        settings.write_text(original, encoding="utf-8")
+
+        with self._fake_zed_platform(settings):
+            configured = install_platform_configs(tmp_path, target="zed")
+
+        # Platform NOT marked configured (we skipped it, didn't overwrite).
+        assert "Zed" not in configured
+
+        # Original file content preserved byte-for-byte.
+        assert settings.read_text(encoding="utf-8") == original
+
+        # User got a clear, actionable message.
+        out = capsys.readouterr().out
+        assert "REFUSING to overwrite" in out
+        assert str(settings) in out
+
+    def test_zed_refuses_when_top_level_is_array(self, tmp_path, capsys):
+        """A file that parses as a valid JSON array (not object) must
+        also be preserved — we expect an object and treating an array as
+        one would lose user data."""
+        settings = tmp_path / "zed" / "settings.json"
+        settings.parent.mkdir(parents=True)
+        settings.write_text('["not", "an", "object"]', encoding="utf-8")
+
+        with self._fake_zed_platform(settings):
+            configured = install_platform_configs(tmp_path, target="zed")
+
+        assert "Zed" not in configured
+        assert settings.read_text() == '["not", "an", "object"]'
+        out = capsys.readouterr().out
+        assert "REFUSING to overwrite" in out
+
+    def test_empty_file_is_treated_as_empty_config(self, tmp_path):
+        """An empty settings file is not malformed — it's just an empty
+        config.  We should proceed and write a fresh one."""
+        settings = tmp_path / "zed" / "settings.json"
+        settings.parent.mkdir(parents=True)
+        settings.write_text("", encoding="utf-8")
+
+        with self._fake_zed_platform(settings):
+            configured = install_platform_configs(tmp_path, target="zed")
+
+        assert "Zed" in configured
+        data = json.loads(settings.read_text())
+        assert "context_servers" in data
+        assert "code-review-graph" in data["context_servers"]
+
+
+class TestInstallSkillsRespectTargetPlatform:
+    """Regression tests for #350: ``install --platform cursor`` previously
+    generated Claude skill markdown at ``<repo>/.claude/skills/``
+    unconditionally.  Cursor (and Windsurf, Zed, etc.) never reads that
+    directory, so the extra files confused users.  Skills should be
+    generated ONLY for Claude-family targets.
+    """
+
+    def _run_install(self, tmp_path, platform: str) -> tuple[bool, str]:
+        """Run ``_handle_init`` with --platform <x> and return
+        (skills_dir_was_created, captured_stdout).
+        """
+        import argparse
+        from io import StringIO
+
+        from code_review_graph import cli as crg_cli
+
+        args = argparse.Namespace(
+            command="install",
+            repo=str(tmp_path),
+            platform=platform,
+            yes=True,
+            dry_run=False,
+            no_skills=False,
+            no_hooks=True,         # we only care about the skills path
+            no_instructions=True,  # skip the instruction-injection prompt
+        )
+
+        buf = StringIO()
+        with patch("sys.stdout", buf):
+            # Silence the prompt machinery entirely by patching input().
+            with patch("builtins.input", return_value="n"):
+                crg_cli._handle_init(args)
+        claude_skills = tmp_path / ".claude" / "skills"
+        return claude_skills.is_dir(), buf.getvalue()
+
+    def test_cursor_install_does_not_create_claude_skills(self, tmp_path):
+        """The bug: ``--platform cursor`` used to create ``.claude/skills/``
+        in the repo, which is only read by Claude Code, not Cursor."""
+        created, output = self._run_install(tmp_path, "cursor")
+        assert not created, (
+            f".claude/skills/ should NOT be created for --platform cursor, "
+            f"but it was.  stdout: {output!r}"
+        )
+        # And we should have told the user we skipped.
+        assert "Skipped generating .claude/skills/" in output
+
+    def test_claude_install_still_creates_skills(self, tmp_path):
+        """Regression guard: the pre-existing Claude path must not
+        regress — ``--platform claude`` still generates skills."""
+        created, _ = self._run_install(tmp_path, "claude")
+        assert created, (
+            ".claude/skills/ should still be created for --platform claude"
+        )
+
+    def test_all_target_still_creates_skills(self, tmp_path):
+        """Regression guard: ``--platform all`` still generates skills
+        so Claude users running the default command are unchanged."""
+        created, _ = self._run_install(tmp_path, "all")
+        assert created
+
+    def test_windsurf_install_does_not_create_claude_skills(self, tmp_path):
+        """Same as cursor: Windsurf does not read .claude/skills/."""
+        created, output = self._run_install(tmp_path, "windsurf")
+        assert not created
+        assert "Skipped generating .claude/skills/" in output
+
+    def test_zed_install_does_not_create_claude_skills(self, tmp_path):
+        """Same as cursor: Zed does not read .claude/skills/."""
+        created, output = self._run_install(tmp_path, "zed")
+        assert not created
+        assert "Skipped generating .claude/skills/" in output
+
+
 class TestKiroPlatform:
     """Tests for Kiro platform support."""