diff --git a/code_review_graph/skills.py b/code_review_graph/skills.py index aba22dc8..43da36fb 100644 --- a/code_review_graph/skills.py +++ b/code_review_graph/skills.py @@ -15,6 +15,7 @@ import re import shutil import stat +import subprocess import sys from pathlib import Path from typing import Any @@ -289,6 +290,41 @@ def _merge_toml_mcp_server( return True +def _strip_trailing_commas_jsonc(text: str) -> str: + """Strip trailing commas before } or ], respecting string boundaries.""" + result = [] + in_string = False + escape = False + i = 0 + while i < len(text): + ch = text[i] + if escape: + result.append(ch) + escape = False + i += 1 + continue + if ch == '\\': + result.append(ch) + escape = True + i += 1 + continue + if ch == '"': + in_string = not in_string + result.append(ch) + i += 1 + continue + if not in_string and ch == ',': + j = i + 1 + while j < len(text) and text[j] in ' \t\n\r': + j += 1 + if j < len(text) and text[j] in '}]': + i += 1 + continue + result.append(ch) + i += 1 + return ''.join(result) + + def install_platform_configs( repo_root: Path, target: str = "all", @@ -347,7 +383,7 @@ def install_platform_configs( # Strip single-line comments and trailing commas (JSONC compat # for editors like Zed that allow non-standard JSON). stripped = re.sub(r'//.*?$', '', raw, flags=re.MULTILINE) - stripped = re.sub(r',(\s*[}\]])', r'\1', stripped) + stripped = _strip_trailing_commas_jsonc(stripped) try: existing = json.loads(stripped) except (json.JSONDecodeError, OSError): @@ -402,11 +438,11 @@ def install_platform_configs( "Use the code-review-graph MCP tools to explore and understand the codebase.\n\n" "### Steps\n\n" "1. Run `list_graph_stats` to see overall codebase metrics.\n" - "2. Run `get_architecture_overview` for high-level community structure.\n" - "3. Use `list_communities` to find major modules, then `get_community` " + "2. Run `get_architecture_overview_tool` for high-level community structure.\n" + "3. Use `list_communities_tool` to find major modules, then `get_community` " "for details.\n" - "4. Use `semantic_search_nodes` to find specific functions or classes.\n" - "5. Use `query_graph` with patterns like `callers_of`, `callees_of`, " + "4. Use `semantic_search_nodes_tool` to find specific functions or classes.\n" + "5. Use `query_graph_tool` with patterns like `callers_of`, `callees_of`, " "`imports_of` to trace relationships.\n" "6. Use `list_flows` and `get_flow` to understand execution paths.\n\n" "### Tips\n\n" @@ -429,11 +465,11 @@ def install_platform_configs( "## Review Changes\n\n" "Perform a thorough, risk-aware code review using the knowledge graph.\n\n" "### Steps\n\n" - "1. Run `detect_changes` to get risk-scored change analysis.\n" - "2. Run `get_affected_flows` to find impacted execution paths.\n" - "3. For each high-risk function, run `query_graph` with " + "1. Run `detect_changes_tool` to get risk-scored change analysis.\n" + "2. Run `get_affected_flows_tool` to find impacted execution paths.\n" + "3. For each high-risk function, run `query_graph_tool` with " 'pattern="tests_for" to check test coverage.\n' - "4. Run `get_impact_radius` to understand the blast radius.\n" + "4. Run `get_impact_radius_tool` to understand the blast radius.\n" "5. For any untested changes, suggest specific test cases.\n\n" "### Output Format\n\n" "Provide findings grouped by risk level (high/medium/low) with:\n" @@ -457,12 +493,12 @@ def install_platform_configs( "## Debug Issue\n\n" "Use the knowledge graph to systematically trace and debug issues.\n\n" "### Steps\n\n" - "1. Use `semantic_search_nodes` to find code related to the issue.\n" - "2. Use `query_graph` with `callers_of` and `callees_of` to trace " + "1. Use `semantic_search_nodes_tool` to find code related to the issue.\n" + "2. Use `query_graph_tool` with `callers_of` and `callees_of` to trace " "call chains.\n" "3. Use `get_flow` to see full execution paths through suspected areas.\n" - "4. Run `detect_changes` to check if recent changes caused the issue.\n" - "5. Use `get_impact_radius` on suspected files to see what else is affected.\n\n" + "4. Run `detect_changes_tool` to check if recent changes caused the issue.\n" + "5. Use `get_impact_radius_tool` on suspected files to see what else is affected.\n\n" "### Tips\n\n" "- Check both callers and callees to understand the full context.\n" "- Look at affected flows to find the entry point that triggers the bug.\n" @@ -489,11 +525,11 @@ def install_platform_configs( '3. For renames, use `refactor_tool` with mode="rename" to preview all ' "affected locations.\n" "4. Use `apply_refactor_tool` with the refactor_id to apply renames.\n" - "5. After changes, run `detect_changes` to verify the refactoring impact.\n\n" + "5. After changes, run `detect_changes_tool` to verify the refactoring impact.\n\n" "### Safety Checks\n\n" "- Always preview before applying (rename mode gives you an edit list).\n" - "- Check `get_impact_radius` before major refactors.\n" - "- Use `get_affected_flows` to ensure no critical paths are broken.\n" + "- Check `get_impact_radius_tool` before major refactors.\n" + "- Use `get_affected_flows_tool` to ensure no critical paths are broken.\n" "- Run `find_large_functions` to identify decomposition targets.\n\n" "## Token Efficiency Rules\n" '- ALWAYS start with `get_minimal_context(task="")` ' @@ -560,6 +596,7 @@ def generate_hooks_config(repo_root: Path) -> dict[str, Any]: { "type": "command", "command": ( + "cat >/dev/null || true; " "git rev-parse --git-dir >/dev/null 2>&1" f" && code-review-graph update --skip-flows" f" --repo {repo_arg}" @@ -577,6 +614,7 @@ def generate_hooks_config(repo_root: Path) -> dict[str, Any]: { "type": "command", "command": ( + "cat >/dev/null || true; " "git rev-parse --git-dir >/dev/null 2>&1" f" && code-review-graph status --repo {repo_arg}" " || echo 'Not a git repo, skipping'" @@ -601,6 +639,7 @@ def generate_codex_hooks_config(repo_root: Path) -> dict[str, Any]: { "type": "command", "command": ( + "cat >/dev/null || true; " "git rev-parse --git-dir >/dev/null 2>&1" " && code-review-graph update --skip-flows" " || true" @@ -618,6 +657,7 @@ def generate_codex_hooks_config(repo_root: Path) -> dict[str, Any]: { "type": "command", "command": ( + "cat >/dev/null || true; " "git rev-parse --git-dir >/dev/null 2>&1" " && code-review-graph status" " || echo 'Not a git repo, skipping'" @@ -635,10 +675,19 @@ def generate_codex_hooks_config(repo_root: Path) -> dict[str, Any]: def install_git_hook(repo_root: Path) -> Path | None: """Install a git pre-commit hook that prints a risk summary before each commit. - Called automatically by ``code-review-graph install`` - Creates ``.git/hooks/pre-commit`` if it doesn't exist, or appends to an - existing one — preserving any hooks already there. Returns None when no - ``.git`` directory is found. + Called automatically by ``code-review-graph install``. + The hooks directory is resolved via ``git rev-parse --git-path hooks`` so + the hook lands where git actually runs it — including linked worktrees + and submodules (where ``.git`` is a file, not a directory) and repos with + ``core.hooksPath`` set (issue #313). ``core.hooksPath`` users with their + own hook manager (husky, pre-commit) may prefer integrating the + ``code-review-graph`` commands into that manager manually instead. + + Creates ``pre-commit`` if it doesn't exist, or appends to an existing + one — the hook is appended, not overwritten, preserving any hooks + already there. Falls back to the legacy ``.git/hooks`` resolution when + git itself is unavailable. Returns None when no hooks directory can be + determined. """ script = """\ #!/bin/sh @@ -650,13 +699,35 @@ def install_git_hook(repo_root: Path) -> Path | None: """ marker = "code-review-graph detect-changes" - git_dir = repo_root / ".git" - if not git_dir.is_dir(): - logger.warning("No .git directory found at %s — skipping git hook install.", repo_root) - return None + hooks_dir: Path | None = None + try: + result = subprocess.run( + ["git", "rev-parse", "--git-path", "hooks"], + capture_output=True, + text=True, + encoding="utf-8", + cwd=str(repo_root), + timeout=10, + stdin=subprocess.DEVNULL, + ) + if result.returncode == 0 and result.stdout.strip(): + # Output is relative to repo_root (".git/hooks", a core.hooksPath + # value such as ".husky") or absolute (linked worktrees). + hooks_dir = repo_root / result.stdout.strip() + except (subprocess.TimeoutExpired, OSError) as exc: + logger.warning("git unavailable (%s); falling back to .git/hooks resolution.", exc) + + if hooks_dir is None: + git_dir = repo_root / ".git" + if not git_dir.is_dir(): + logger.warning( + "No git hooks directory found at %s — skipping git hook install.", repo_root + ) + return None + hooks_dir = git_dir / "hooks" - hook_path = git_dir / "hooks" / "pre-commit" - hook_path.parent.mkdir(exist_ok=True) + hook_path = hooks_dir / "pre-commit" + hook_path.parent.mkdir(parents=True, exist_ok=True) if hook_path.exists(): existing = hook_path.read_text(encoding="utf-8") @@ -791,11 +862,11 @@ def install_codex_hooks(repo_root: Path) -> Path: ### When to use graph tools FIRST -- **Exploring code**: `semantic_search_nodes` or `query_graph` instead of Grep -- **Understanding impact**: `get_impact_radius` instead of manually tracing imports -- **Code review**: `detect_changes` + `get_review_context` instead of reading entire files -- **Finding relationships**: `query_graph` with callers_of/callees_of/imports_of/tests_for -- **Architecture questions**: `get_architecture_overview` + `list_communities` +- **Exploring code**: `semantic_search_nodes_tool` or `query_graph_tool` instead of Grep +- **Understanding impact**: `get_impact_radius_tool` instead of manually tracing imports +- **Code review**: `detect_changes_tool` + `get_review_context_tool` instead of reading entire files +- **Finding relationships**: `query_graph_tool` with callers_of/callees_of/imports_of/tests_for +- **Architecture questions**: `get_architecture_overview_tool` + `list_communities_tool` Fall back to Grep/Glob/Read **only** when the graph doesn't cover what you need. @@ -803,21 +874,21 @@ def install_codex_hooks(repo_root: Path) -> Path: | Tool | Use when | | ------ | ---------- | -| `detect_changes` | Reviewing code changes — gives risk-scored analysis | -| `get_review_context` | Need source snippets for review — token-efficient | -| `get_impact_radius` | Understanding blast radius of a change | -| `get_affected_flows` | Finding which execution paths are impacted | -| `query_graph` | Tracing callers, callees, imports, tests, dependencies | -| `semantic_search_nodes` | Finding functions/classes by name or keyword | -| `get_architecture_overview` | Understanding high-level codebase structure | +| `detect_changes_tool` | Reviewing code changes — gives risk-scored analysis | +| `get_review_context_tool` | Need source snippets for review — token-efficient | +| `get_impact_radius_tool` | Understanding blast radius of a change | +| `get_affected_flows_tool` | Finding which execution paths are impacted | +| `query_graph_tool` | Tracing callers, callees, imports, tests, dependencies | +| `semantic_search_nodes_tool` | Finding functions/classes by name or keyword | +| `get_architecture_overview_tool` | Understanding high-level codebase structure | | `refactor_tool` | Planning renames, finding dead code | ### Workflow 1. The graph auto-updates on file changes (via hooks). -2. Use `detect_changes` for code review. -3. Use `get_affected_flows` to understand impact. -4. Use `query_graph` pattern=\"tests_for\" to check coverage. +2. Use `detect_changes_tool` for code review. +3. Use `get_affected_flows_tool` to understand impact. +4. Use `query_graph_tool` pattern=\"tests_for\" to check coverage. """ # Copilot-specific instruction file content: uses VS Code tool references and @@ -840,11 +911,11 @@ def install_codex_hooks(repo_root: Path) -> Path: ### When to use graph tools FIRST -- **Exploring code**: `semantic_search_nodes` or `query_graph` -- **Understanding impact**: `get_impact_radius` -- **Code review**: `detect_changes` + `get_review_context` -- **Finding relationships**: `query_graph` callers_of/callees_of -- **Architecture questions**: `get_architecture_overview` +- **Exploring code**: `semantic_search_nodes_tool` or `query_graph_tool` +- **Understanding impact**: `get_impact_radius_tool` +- **Code review**: `detect_changes_tool` + `get_review_context_tool` +- **Finding relationships**: `query_graph_tool` callers_of/callees_of +- **Architecture questions**: `get_architecture_overview_tool` Fall back to file/search tools **only** when the graph doesn't cover what you need. @@ -853,21 +924,21 @@ def install_codex_hooks(repo_root: Path) -> Path: | Tool | Use when | | ------ | ---------- | -| `detect_changes` | Risk-scored change analysis | -| `get_review_context` | Token-efficient source snippets | -| `get_impact_radius` | Blast radius of a change | -| `get_affected_flows` | Impacted execution paths | -| `query_graph` | Trace callers, callees, imports, tests | -| `semantic_search_nodes` | Find functions/classes by keyword | -| `get_architecture_overview` | High-level structure | +| `detect_changes_tool` | Risk-scored change analysis | +| `get_review_context_tool` | Token-efficient source snippets | +| `get_impact_radius_tool` | Blast radius of a change | +| `get_affected_flows_tool` | Impacted execution paths | +| `query_graph_tool` | Trace callers, callees, imports, tests | +| `semantic_search_nodes_tool` | Find functions/classes by keyword | +| `get_architecture_overview_tool` | High-level structure | | `refactor_tool` | Rename planning, dead code | ### Workflow 1. The graph auto-updates on file changes (via hooks). -2. Use `detect_changes` for code review. -3. Use `get_affected_flows` to understand impact. -4. Use `query_graph` pattern=\"tests_for\" to check coverage. +2. Use `detect_changes_tool` for code review. +3. Use `get_affected_flows_tool` to understand impact. +4. Use `query_graph_tool` pattern=\"tests_for\" to check coverage. """ # Maps instruction file path → (marker, section) for files that need content