Skip to content

fix(skills): string-aware trailing comma stripping for JSONC #571

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
itxaiohanglover wants to merge 1 commit into from
Closed

fix(skills): string-aware trailing comma stripping for JSONC #571

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
183 changes: 127 additions & 56 deletions code_review_graph/skills.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import re
import shutil
import stat
import subprocess
import sys
from pathlib import Path
from typing import Any
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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"
Expand All @@ -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"
Expand All @@ -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"
Expand All @@ -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="<your task>")` '
Expand Down Expand Up @@ -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}"
Expand All @@ -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'"
Expand All @@ -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"
Expand All @@ -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'"
Expand All @@ -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
Expand All @@ -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")
Expand Down Expand Up @@ -791,33 +862,33 @@ 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.

### Key Tools

| 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
Expand All @@ -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.
Expand All @@ -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
Expand Down