diff --git a/code_review_graph/cli.py b/code_review_graph/cli.py index 6e9349b6..ecd1242c 100644 --- a/code_review_graph/cli.py +++ b/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 + # ``/.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 ``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() diff --git a/code_review_graph/skills.py b/code_review_graph/skills.py index 83414a90..e8ce90c7 100644 --- a/code_review_graph/skills.py +++ b/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, []) diff --git a/tests/test_cli.py b/tests/test_cli.py index d01186ff..36f89edc 100644 --- a/tests/test_cli.py +++ b/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}" + ) diff --git a/tests/test_skills.py b/tests/test_skills.py index a55a61aa..eb53a903 100644 --- a/tests/test_skills.py +++ b/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 ``/.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 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."""