From 5b914a45b6ab988b52476efa807c001466779376 Mon Sep 17 00:00:00 2001 From: Tirth Kanani Date: Wed, 19 Aug 2026 19:47:02 +0100 Subject: [PATCH 1/4] docs(instructions): use the graph to narrow scope, the source to verify The generated instruction section told agents to ALWAYS use the graph before Grep/Glob/Read and to fall back to file search only when the graph did not cover the need. In practice that made models act on graph summaries, skip the implementation, and produce shallower work. A zero result can also mean "not indexed" rather than "does not exist". Every platform instruction file now carries the same short guardrails: narrow scope with the graph then read the source, read the implementation and its tests before a non-trivial change, verify the exact source for behavior, database, migration, retry, fallback, recovery and compatibility code, prefer the source when the two disagree, and treat an empty result as possibly unindexed. The intro and the "fall back only when" line were tightened to pay for most of the space, so the injected section grows by 6 lines (38 to 44) for CLAUDE.md and 5 lines (46 to 51) for Copilot. The intro and guardrails are shared constants so all 9 targets stay identical. The four skill templates no longer say ALWAYS start with get_minimal_context_tool, and each gained one line about reading the implementation before changing code. Bundled skills/ regenerated to match. CONTRIBUTING.md gains a platform target section stating what a new AI-tool install target must include before review: official MCP config docs, the exact path and schema, an entry through the existing PLATFORMS table, preserved unrelated settings, byte-idempotent reinstall, working uninstall, lifecycle tests matching the existing ones, and evidence from a real released client. Requests no maintainer can test may be declined. Fixes #314 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012fHfGDiZedoxjpKzanHri3 --- CHANGELOG.md | 8 ++++ CONTRIBUTING.md | 41 ++++++++++++++++++++ code_review_graph/skills.py | 64 +++++++++++++++++++------------- skills/debug-issue/SKILL.md | 3 +- skills/explore-codebase/SKILL.md | 3 +- skills/refactor-safely/SKILL.md | 3 +- skills/review-changes/SKILL.md | 3 +- tests/test_skills.py | 58 +++++++++++++++++++++++++++++ 8 files changed, 154 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f13155327..1d51b9600 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,14 @@ ### Fixed +- The generated instruction sections no longer tell agents to always use the + graph before reading source and to fall back to file search only when the + graph misses. Every platform instruction file now carries the same short + guardrails: narrow scope with the graph, read the implementation and its + tests before a non-trivial change, prefer the source when the two disagree, + and treat an empty graph result as possibly unindexed rather than absent. + CONTRIBUTING.md also states what a new platform target must include before + it will be reviewed (#314). - C# receiver calls (`Service.StaticCall()`, `obj.Method()`, `obj?.Method()`) now resolve to canonical method nodes using receiver-type and namespace evidence recorded at parse time, so `callers_of`, `get_impact_radius`, and diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d4b006422..14d235d34 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -80,6 +80,47 @@ If you just need a language for your own repo, you may not need to contribute at 3. Add a sample fixture file in `tests/fixtures/` 4. Add parsing tests in `tests/test_multilang.py` +## Adding a Platform Target + +Every supported AI tool is permanent maintenance surface. Its config path, schema, install merge, +uninstall, and tests all have to keep working on every release. Some existing targets were merged +without any evidence that the integration worked in a released client, and those are the ones that +break. New targets are held to the bar below. + +Start with a platform request issue (https://github.com/tirth8205/code-review-graph/issues/new/choose) +so the client can be discussed before anyone writes code. A pull request that adds a platform will +not be reviewed until it includes all of the following. + +1. A link to the platform's official MCP configuration documentation. Blog posts, forum replies, + and screenshots of a settings dialog are not enough. +2. The exact config file path and the exact schema of a server entry, including which top-level key + holds the servers, whether that value is an object or an array, and whether a `type` field is + required. +3. The entry added through the existing `PLATFORMS` table in `code_review_graph/skills.py`, plus + `_PLATFORM_CHOICES` in `code_review_graph/cli.py`. Use the fields already there: `name`, + `config_path`, `key`, `detect`, `format`, `needs_type`, and where needed `legacy_keys`, + `server_type`, `entry_fields`. If the client needs something the table cannot express, say so in + the pull request and explain why, rather than adding a bespoke code path beside it. +4. Preservation of unrelated user settings. Install must merge only the `code-review-graph` server + entry and leave every other server, key, and top-level setting intact. If the file cannot be + parsed, install must skip it rather than rewrite it. +5. A byte-idempotent reinstall. Running install twice must leave the config file and any generated + instruction file byte for byte identical. +6. A working uninstall in `code_review_graph/uninstall.py` that removes only what install added, + including any legacy keys, and leaves the rest of the file untouched. +7. Lifecycle tests matching the existing ones: an install, reinstall, and uninstall test in + `tests/test_cli_install.py` shaped like `test_copilot_cli_install_reinstall_uninstall_lifecycle`, + and a passing run of the all-platforms sweep in `tests/test_uninstall.py` + (`test_uninstall_removes_mcp_entry_for_every_current_platform_spec`), which every new entry is + automatically subject to. +8. Evidence from a real released client: a screenshot or transcript of an actual session in that + client where a code-review-graph tool is invoked and returns a result. A rendered image of text, + a mockup, or a description of what should happen is not evidence. + +If no maintainer can install and run the client, the request may be declined or left open until +someone who uses it is willing to own it and respond when it breaks. An existing target may also be +removed if it breaks and nobody steps up to fix it. + ## Reporting Issues - Open an issue via the issue forms: https://github.com/tirth8205/code-review-graph/issues/new/choose (bug report, feature request, or platform request — blank issues are disabled) diff --git a/code_review_graph/skills.py b/code_review_graph/skills.py index c529384c9..8f2c15554 100644 --- a/code_review_graph/skills.py +++ b/code_review_graph/skills.py @@ -891,12 +891,14 @@ def _record_configured(key: str, plat: dict[str, Any]) -> None: "- Use `children_of` on a file to see all its functions and classes.\n" "- Use `find_large_functions_tool` to identify complex code.\n\n" "## Token Efficiency Rules\n" - '- ALWAYS start with `get_minimal_context_tool(task="")` ' - "before any other graph tool.\n" + '- Start with `get_minimal_context_tool(task="")` ' + "before other graph tools.\n" '- Use `detail_level="minimal"` on all calls. Only escalate to ' '"standard" when minimal is insufficient.\n' "- Target: complete any review/debug/refactor task in ≤5 tool calls " - "and ≤800 total output tokens." + "and ≤800 total output tokens.\n" + "- Read the implementation and its tests before changing code. The graph " + "narrows scope; it does not replace the source." ), }, "review-changes.md": { @@ -919,12 +921,14 @@ def _record_configured(key: str, plat: dict[str, Any]) -> None: "- Suggested improvements\n" "- Overall merge recommendation\n\n" "## Token Efficiency Rules\n" - '- ALWAYS start with `get_minimal_context_tool(task="")` ' - "before any other graph tool.\n" + '- Start with `get_minimal_context_tool(task="")` ' + "before other graph tools.\n" '- Use `detail_level="minimal"` on all calls. Only escalate to ' '"standard" when minimal is insufficient.\n' "- Target: complete any review/debug/refactor task in ≤5 tool calls " - "and ≤800 total output tokens." + "and ≤800 total output tokens.\n" + "- Read the implementation and its tests before changing code. The graph " + "narrows scope; it does not replace the source." ), }, "debug-issue.md": { @@ -945,12 +949,14 @@ def _record_configured(key: str, plat: dict[str, Any]) -> None: "- Look at affected flows to find the entry point that triggers the bug.\n" "- Recent changes are the most common source of new issues.\n\n" "## Token Efficiency Rules\n" - '- ALWAYS start with `get_minimal_context_tool(task="")` ' - "before any other graph tool.\n" + '- Start with `get_minimal_context_tool(task="")` ' + "before other graph tools.\n" '- Use `detail_level="minimal"` on all calls. Only escalate to ' '"standard" when minimal is insufficient.\n' "- Target: complete any review/debug/refactor task in ≤5 tool calls " - "and ≤800 total output tokens." + "and ≤800 total output tokens.\n" + "- Read the implementation and its tests before changing code. The graph " + "narrows scope; it does not replace the source." ), }, "refactor-safely.md": { @@ -973,12 +979,14 @@ def _record_configured(key: str, plat: dict[str, Any]) -> None: "- Use `get_affected_flows_tool` to ensure no critical paths are broken.\n" "- Run `find_large_functions_tool` to identify decomposition targets.\n\n" "## Token Efficiency Rules\n" - '- ALWAYS start with `get_minimal_context_tool(task="")` ' - "before any other graph tool.\n" + '- Start with `get_minimal_context_tool(task="")` ' + "before other graph tools.\n" '- Use `detail_level="minimal"` on all calls. Only escalate to ' '"standard" when minimal is insufficient.\n' "- Target: complete any review/debug/refactor task in ≤5 tool calls " - "and ≤800 total output tokens." + "and ≤800 total output tokens.\n" + "- Read the implementation and its tests before changing code. The graph " + "narrows scope; it does not replace the source." ), }, } @@ -1332,14 +1340,25 @@ def install_codex_hooks(repo_root: Path) -> Path: _CLAUDE_MD_SECTION_MARKER = "" +# Shared across every platform instruction file so the wording stays identical. +_INSTRUCTION_INTRO = """**This project has a knowledge graph. Start with the code-review-graph +MCP tools to narrow scope, then read the source.** The graph is cheaper than scanning files and +gives you structural context (callers, dependents, test coverage) that file search cannot.""" + +_INSTRUCTION_GUARDRAILS = """### Verify in the source + +- Narrow scope with the graph, then read the source. Do not change code from graph output alone. +- For any non-trivial change, read the implementation and the relevant tests before concluding. +- Verify the exact source when touching behavior, database logic, migrations, retries, fallbacks, + recovery, or compatibility code. +- When the graph and the source disagree, the source wins. The graph may be stale or may not + model that relationship. +- An empty graph result can mean "not indexed" or "not statically visible", not "does not exist".""" + _CLAUDE_MD_SECTION = f"""{_CLAUDE_MD_SECTION_MARKER} ## MCP Tools: code-review-graph -**IMPORTANT: This project has a knowledge graph. ALWAYS use the -code-review-graph MCP tools BEFORE using Grep/Glob/Read to explore -the codebase.** The graph is faster, cheaper (fewer tokens), and gives -you structural context (callers, dependents, test coverage) that file -scanning cannot. +{_INSTRUCTION_INTRO} ### When to use graph tools FIRST @@ -1349,7 +1368,7 @@ def install_codex_hooks(repo_root: Path) -> Path: - **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. +{_INSTRUCTION_GUARDRAILS} ### Key Tools @@ -1384,11 +1403,7 @@ def install_codex_hooks(repo_root: Path) -> Path: {_CLAUDE_MD_SECTION_MARKER} ## MCP Tools: code-review-graph -**IMPORTANT: This project has a knowledge graph. ALWAYS use the -code-review-graph MCP tools BEFORE using file/search tools to -explore the codebase.** The graph is faster, cheaper (fewer -tokens), and gives you structural context (callers, dependents, -test coverage) that file scanning cannot. +{_INSTRUCTION_INTRO} ### When to use graph tools FIRST @@ -1398,8 +1413,7 @@ def install_codex_hooks(repo_root: Path) -> Path: - **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. +{_INSTRUCTION_GUARDRAILS} ### Key Tools diff --git a/skills/debug-issue/SKILL.md b/skills/debug-issue/SKILL.md index a0fc76935..124844cf1 100644 --- a/skills/debug-issue/SKILL.md +++ b/skills/debug-issue/SKILL.md @@ -22,6 +22,7 @@ Use the knowledge graph to systematically trace and debug issues. - Recent changes are the most common source of new issues. ## Token Efficiency Rules -- ALWAYS start with `get_minimal_context_tool(task="")` before any other graph tool. +- Start with `get_minimal_context_tool(task="")` before other graph tools. - Use `detail_level="minimal"` on all calls. Only escalate to "standard" when minimal is insufficient. - Target: complete any review/debug/refactor task in ≤5 tool calls and ≤800 total output tokens. +- Read the implementation and its tests before changing code. The graph narrows scope; it does not replace the source. diff --git a/skills/explore-codebase/SKILL.md b/skills/explore-codebase/SKILL.md index 74dec19b2..67eca1ac1 100644 --- a/skills/explore-codebase/SKILL.md +++ b/skills/explore-codebase/SKILL.md @@ -23,6 +23,7 @@ Use the code-review-graph MCP tools to explore and understand the codebase. - Use `find_large_functions_tool` to identify complex code. ## Token Efficiency Rules -- ALWAYS start with `get_minimal_context_tool(task="")` before any other graph tool. +- Start with `get_minimal_context_tool(task="")` before other graph tools. - Use `detail_level="minimal"` on all calls. Only escalate to "standard" when minimal is insufficient. - Target: complete any review/debug/refactor task in ≤5 tool calls and ≤800 total output tokens. +- Read the implementation and its tests before changing code. The graph narrows scope; it does not replace the source. diff --git a/skills/refactor-safely/SKILL.md b/skills/refactor-safely/SKILL.md index 410b5c397..4803ba04e 100644 --- a/skills/refactor-safely/SKILL.md +++ b/skills/refactor-safely/SKILL.md @@ -23,6 +23,7 @@ Use the knowledge graph to plan and execute refactoring with confidence. - Run `find_large_functions_tool` to identify decomposition targets. ## Token Efficiency Rules -- ALWAYS start with `get_minimal_context_tool(task="")` before any other graph tool. +- Start with `get_minimal_context_tool(task="")` before other graph tools. - Use `detail_level="minimal"` on all calls. Only escalate to "standard" when minimal is insufficient. - Target: complete any review/debug/refactor task in ≤5 tool calls and ≤800 total output tokens. +- Read the implementation and its tests before changing code. The graph narrows scope; it does not replace the source. diff --git a/skills/review-changes/SKILL.md b/skills/review-changes/SKILL.md index c62810d2b..0c2234bc6 100644 --- a/skills/review-changes/SKILL.md +++ b/skills/review-changes/SKILL.md @@ -24,6 +24,7 @@ Provide findings grouped by risk level (high/medium/low) with: - Overall merge recommendation ## Token Efficiency Rules -- ALWAYS start with `get_minimal_context_tool(task="")` before any other graph tool. +- Start with `get_minimal_context_tool(task="")` before other graph tools. - Use `detail_level="minimal"` on all calls. Only escalate to "standard" when minimal is insufficient. - Target: complete any review/debug/refactor task in ≤5 tool calls and ≤800 total output tokens. +- Read the implementation and its tests before changing code. The graph narrows scope; it does not replace the source. diff --git a/tests/test_skills.py b/tests/test_skills.py index 5aac357d7..b2c0ed748 100644 --- a/tests/test_skills.py +++ b/tests/test_skills.py @@ -801,6 +801,64 @@ def test_codebuddy_writes_only_codebuddy_md_and_is_idempotent(self, tmp_path): assert not (tmp_path / "AGENTS.md").exists() +class TestInstructionGuardrails: + """Every generated instruction file must carry the source-verification guardrails. + + Regression test for #314: the generated text used to tell agents to ALWAYS use + the graph before reading source and to fall back to file search ONLY when the + graph did not cover the need, which made models act on graph summaries alone. + """ + + # Each guardrail is asserted by a fragment short enough to survive rewrapping. + GUARDRAIL_FRAGMENTS = ( + "Do not change code from graph output alone", + "read the implementation and the relevant tests before concluding", + "migrations, retries, fallbacks", + "the source wins", + 'can mean "not indexed" or "not statically visible"', + ) + + @staticmethod + def _instruction_files(tmp_path: Path) -> dict[str, str]: + inject_claude_md(tmp_path) + inject_platform_instructions(tmp_path, target="all") + names = ["CLAUDE.md", *skills_module._PLATFORM_INSTRUCTION_FILES] + return { + name: (tmp_path / name).read_text(encoding="utf-8") for name in names + } + + def test_every_instruction_file_has_all_guardrails(self, tmp_path): + written = self._instruction_files(tmp_path) + assert len(written) == 9 + for name, content in written.items(): + assert "### Verify in the source" in content, name + for fragment in self.GUARDRAIL_FRAGMENTS: + assert fragment in content, f"{name} is missing: {fragment}" + + def test_no_instruction_file_claims_the_graph_replaces_source(self, tmp_path): + for name, content in self._instruction_files(tmp_path).items(): + assert "ALWAYS use the" not in content, name + assert "**only** when the graph" not in content, name + + def test_shared_guardrail_text_is_identical_across_platforms(self, tmp_path): + written = self._instruction_files(tmp_path) + for name, content in written.items(): + assert skills_module._INSTRUCTION_GUARDRAILS in content, name + assert skills_module._INSTRUCTION_INTRO in content, name + + def test_guardrails_stay_small(self): + """The section ships in every user's context, so cap its growth.""" + assert skills_module._INSTRUCTION_GUARDRAILS.count("\n") + 1 <= 9 + assert skills_module._CLAUDE_MD_SECTION.count("\n") <= 46 + assert skills_module._COPILOT_SECTION.count("\n") <= 53 + + def test_skill_templates_do_not_demand_graph_only_work(self): + for filename, skill in skills_module._SKILLS.items(): + body = skill["body"] + assert "ALWAYS start with" not in body, filename + assert "Read the implementation and its tests before changing code." in body, filename + + class TestCodeBuddyPlatform: def test_platform_uses_official_project_mcp_contract(self): assert "codebuddy" in PLATFORMS From 060fc1ae785954106fee90e6782fae671db65ac9 Mon Sep 17 00:00:00 2001 From: Tirth Kanani Date: Wed, 19 Aug 2026 20:00:24 +0100 Subject: [PATCH 2/4] fix(install): upgrade a stale instruction block instead of skipping the file _inject_instructions only checked whether the opening marker was present. Anyone who installed at an earlier version kept that release's text forever and reinstalling was a no-op, so the guardrails added in the previous commit would only ever reach brand-new installs. Same upgrade-path failure as #558. Generated sections now end with so the managed block has real boundaries. Blocks written before that marker existed have no closing boundary, so they are found by matching their full recorded text rather than by guessing where they stop, and only text that exactly equals a block this project generated is ever rewritten. Anything the user wrote around the block survives byte for byte, and duplicate stale blocks from repeat installs collapse to one. _legacy_instructions.py records the 10 past block texts, recovered by walking git history twice: an AST evaluation of every past revision of skills.py, and a scan of the instruction files this repo ships, which caught blocks a markdown formatter rewrote after generation. The list is append-only. _inject_instructions now returns created, updated, unchanged or conflict. Identical content stays a no-op and does not rewrite the file. A block that matches nothing known was hand-edited: it is left untouched and reported, so install names the file instead of silently doing nothing. inject_platform_ instructions keeps its list-of-filenames contract; the new inject_instruction_files carries the outcomes, and the install command and its dry-run preview both use them. Refs #314, #558 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012fHfGDiZedoxjpKzanHri3 --- CHANGELOG.md | 9 + code_review_graph/_legacy_instructions.py | 455 ++++++++++++++++++++++ code_review_graph/cli.py | 72 ++-- code_review_graph/skills.py | 128 +++++- pyproject.toml | 1 + tests/test_cli_install.py | 8 +- tests/test_skills.py | 152 ++++++++ 7 files changed, 779 insertions(+), 46 deletions(-) create mode 100644 code_review_graph/_legacy_instructions.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 1d51b9600..4fd6a1eaf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,15 @@ and treat an empty graph result as possibly unindexed rather than absent. CONTRIBUTING.md also states what a new platform target must include before it will be reviewed (#314). +- Reinstalling now upgrades an instruction section written by an older release + instead of skipping the file because the opening marker was already there, + which is what made every wording fix invisible to existing users. Generated + sections carry a closing marker so the block has real boundaries; blocks from + before that marker existed are matched by their exact recorded text, so + anything written around them survives byte for byte. A section someone edited + by hand is left alone and named in the install output instead of being + overwritten, and `install` now reports created, updated and left-alone files + separately (#314, #558). - C# receiver calls (`Service.StaticCall()`, `obj.Method()`, `obj?.Method()`) now resolve to canonical method nodes using receiver-type and namespace evidence recorded at parse time, so `callers_of`, `get_impact_radius`, and diff --git a/code_review_graph/_legacy_instructions.py b/code_review_graph/_legacy_instructions.py new file mode 100644 index 000000000..713220604 --- /dev/null +++ b/code_review_graph/_legacy_instructions.py @@ -0,0 +1,455 @@ +"""Verbatim copies of instruction sections shipped by earlier releases. + +``skills._inject_instructions`` upgrades a managed block in place, and it only +rewrites text that exactly equals a block this project generated. Blocks written +before the end marker existed have no closing boundary, so matching their full +text is the only way to find them without guessing where they stop. That is what +this module stores. + +The list is append-only. Dropping an entry means anyone still carrying that block +stops being upgraded and gets reported as hand-edited instead. Entries were +recovered by walking git history twice: an AST evaluation of every past revision +of ``code_review_graph/skills.py``, and a scan of the instruction files this repo +ships, which caught blocks a markdown formatter rewrote after generation. +""" + +from __future__ import annotations + +# shipped in .github/code-review-graph.instruction.md@997dcf49 (2062 chars) +_SECTION_01 = """--- +applyTo: '**' +description: Use code-review-graph MCP tools for token-efficient codebase exploration and code review instead of built-in file/search tools. +--- + + +## MCP Tools: code-review-graph + +**IMPORTANT: This project has a knowledge graph. ALWAYS use the +code-review-graph MCP tools BEFORE using #tool:read/readFile #tool:search/fileSearch #tool:search/textSearch to explore +the codebase.** The graph is faster, cheaper (fewer tokens), and gives +you structural context (callers, dependents, test coverage) that file +scanning cannot. + +### When to use graph tools FIRST + +- **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_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_tool` for code review. +3. Use `get_affected_flows_tool` to understand impact. +4. Use `query_graph_tool` pattern="tests_for" to check coverage. +""" + +# skills.py _COPILOT_SECTION as of 9f029e01 (2049 chars) +_SECTION_02 = """--- +applyTo: '**' +description: Use code-review-graph MCP tools for token-efficient codebase exploration and code review instead of built-in file/search tools. +--- + + +## MCP Tools: code-review-graph + +**IMPORTANT: This project has a knowledge graph. ALWAYS use the +code-review-graph MCP tools BEFORE using #tool:read/readFile #tool:search/fileSearch #tool:search/textSearch to explore +the codebase.** The graph is faster, cheaper (fewer tokens), and gives +you structural context (callers, dependents, test coverage) that file +scanning cannot. + +### When to use graph tools FIRST + +- **Exploring code**: `semantic_search_nodes` or `query_graph` instead of #tool:search/fileSearch +- **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` + +Fall back to #tool:read/readFile, #tool:search/fileSearch, or #tool:search/textSearch **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 | +| `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. +""" + +# skills.py _CLAUDE_MD_SECTION as of 6e454683 (1845 chars) +_SECTION_03 = """ +## MCP Tools: code-review-graph + +**IMPORTANT: This project has a knowledge graph. ALWAYS use the +code-review-graph MCP tools BEFORE using Grep/Glob/Read to explore +the codebase.** The graph is faster, cheaper (fewer tokens), and gives +you structural context (callers, dependents, test coverage) that file +scanning cannot. + +### When to use graph tools FIRST + +- **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_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_tool` for code review. +3. Use `get_affected_flows_tool` to understand impact. +4. Use `query_graph_tool` pattern="tests_for" to check coverage. +""" + +# shipped in CLAUDE.md@039f231a (1841 chars) +_SECTION_04 = """ +## MCP Tools: code-review-graph + +**IMPORTANT: This project has a knowledge graph. ALWAYS use the +code-review-graph MCP tools BEFORE using Grep/Glob/Read to explore +the codebase.** The graph is faster, cheaper (fewer tokens), and gives +you structural context (callers, dependents, test coverage) that file +scanning cannot. + +### When to use graph tools FIRST + +- **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_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_tool` for code review. +3. Use `get_affected_flows_tool` to understand impact. +4. Use `query_graph_tool` pattern="tests_for" to check coverage. +""" + +# skills.py _CLAUDE_MD_SECTION as of 7990fc8f (1755 chars) +_SECTION_05 = """ +## MCP Tools: code-review-graph + +**IMPORTANT: This project has a knowledge graph. ALWAYS use the +code-review-graph MCP tools BEFORE using Grep/Glob/Read to explore +the codebase.** The graph is faster, cheaper (fewer tokens), and gives +you structural context (callers, dependents, test coverage) that file +scanning cannot. + +### 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` + +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 | +| `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. +""" + +# skills.py _CLAUDE_MD_SECTION as of 779b3761 (1751 chars) +_SECTION_06 = """ +## MCP Tools: code-review-graph + +**IMPORTANT: This project has a knowledge graph. ALWAYS use the +code-review-graph MCP tools BEFORE using Grep/Glob/Read to explore +the codebase.** The graph is faster, cheaper (fewer tokens), and gives +you structural context (callers, dependents, test coverage) that file +scanning cannot. + +### 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` + +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 | +| `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. +""" + +# skills.py _CLAUDE_MD_SECTION as of df5f3761 (1751 chars) +_SECTION_07 = """ +## MCP Tools: code-review-graph + +**IMPORTANT: This project has a knowledge graph. ALWAYS use the code-review-graph MCP tools BEFORE using Grep/Glob/Read to explore the codebase.** The graph is faster, cheaper (fewer tokens), and gives you structural context (callers, dependents, test coverage) that file scanning cannot. + +### 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` + +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 | +| `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. +""" + +# skills.py _COPILOT_SECTION as of 6e454683 (1715 chars) +_SECTION_08 = """--- +applyTo: '**' +description: >- + Use code-review-graph MCP tools for token-efficient + codebase exploration and code review. +--- + + +## MCP Tools: code-review-graph + +**IMPORTANT: This project has a knowledge graph. ALWAYS use the +code-review-graph MCP tools BEFORE using file/search tools to +explore the codebase.** The graph is faster, cheaper (fewer +tokens), and gives you structural context (callers, dependents, +test coverage) that file scanning cannot. + +### When to use graph tools FIRST + +- **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. + +### Key Tools + +| Tool | Use when | +| ------ | ---------- | +| `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_tool` for code review. +3. Use `get_affected_flows_tool` to understand impact. +4. Use `query_graph_tool` pattern="tests_for" to check coverage. +""" + +# skills.py _COPILOT_SECTION as of 7990fc8f (1630 chars) +_SECTION_09 = """--- +applyTo: '**' +description: >- + Use code-review-graph MCP tools for token-efficient + codebase exploration and code review. +--- + + +## MCP Tools: code-review-graph + +**IMPORTANT: This project has a knowledge graph. ALWAYS use the +code-review-graph MCP tools BEFORE using file/search tools to +explore the codebase.** The graph is faster, cheaper (fewer +tokens), and gives you structural context (callers, dependents, +test coverage) that file scanning cannot. + +### 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` + +Fall back to file/search tools **only** when the graph doesn't +cover what you need. + +### Key Tools + +| 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 | +| `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. +""" + +# skills.py _CLAUDE_MD_SECTION as of a14e6029 (1377 chars) +_SECTION_10 = """ +## MCP Tools: code-review-graph + +This project uses **code-review-graph** for structural code analysis via MCP. + +### Available Tools + +| Tool | Description | +|------|-------------| +| `build_or_update_graph` | Build or incrementally update the knowledge graph | +| `detect_changes` | Risk-scored change impact analysis for code review | +| `get_impact_radius` | Blast radius from changed files | +| `get_review_context` | Focused review context with source snippets | +| `get_affected_flows` | Find execution flows affected by changes | +| `query_graph` | Predefined graph queries (callers, callees, imports, tests) | +| `semantic_search_nodes` | Search by name or semantic similarity | +| `list_flows` / `get_flow` | Explore execution flows | +| `list_communities` / `get_community` | Explore code communities | +| `get_architecture_overview` | High-level architecture from communities | +| `find_large_functions` | Find oversized functions/classes | +| `refactor_tool` / `apply_refactor_tool` | Graph-powered refactoring | +| `list_graph_stats` | Codebase metrics | +| `embed_graph` | Compute vector embeddings for semantic search | + +### 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. +""" + +# Longest first: a shorter variant must never shadow a longer one that +# contains it. ``skills`` relies on this ordering when it picks a match. +LEGACY_INSTRUCTION_SECTIONS: tuple[str, ...] = ( + _SECTION_01, + _SECTION_02, + _SECTION_03, + _SECTION_04, + _SECTION_05, + _SECTION_06, + _SECTION_07, + _SECTION_08, + _SECTION_09, + _SECTION_10, +) diff --git a/code_review_graph/cli.py b/code_review_graph/cli.py index aecf0013d..4cff13f7c 100644 --- a/code_review_graph/cli.py +++ b/code_review_graph/cli.py @@ -162,30 +162,43 @@ def _instruction_files_to_modify( """Return the list of instruction files that ``install`` would write or modify, given the current state of the repo and the selected platform target. Used for the dry-run / confirm preview (#173). + + A file holding a section from an older release is listed as ``(update)``: + install replaces that block in place rather than leaving it stale (#314). """ - from .skills import _CLAUDE_MD_SECTION_MARKER, _PLATFORM_INSTRUCTION_FILES + from .skills import ( + _CLAUDE_MD_SECTION, + _CLAUDE_MD_SECTION_MARKER, + _COPILOT_SECTION, + _PLATFORM_INSTRUCTION_CUSTOM_SECTIONS, + _PLATFORM_INSTRUCTION_FILES, + _upgrade_managed_block, + ) targets: list[str] = [] + def _describe(filename: str, path: Path, section: str) -> None: + if not path.exists(): + targets.append(f"{filename} (new)") + return + content = path.read_text(encoding="utf-8", errors="replace") + if _CLAUDE_MD_SECTION_MARKER not in content: + targets.append(f"{filename} (append)") + elif _upgrade_managed_block(content, section) is not None: + targets.append(f"{filename} (update)") + if target in ("claude", "all"): - claude_md = repo_root / "CLAUDE.md" - if claude_md.exists(): - content = claude_md.read_text(encoding="utf-8") - if _CLAUDE_MD_SECTION_MARKER not in content: - targets.append("CLAUDE.md (append)") - else: - targets.append("CLAUDE.md (new)") + _describe("CLAUDE.md", repo_root / "CLAUDE.md", _CLAUDE_MD_SECTION) for filename, owners in _PLATFORM_INSTRUCTION_FILES.items(): if target != "all" and target not in owners: continue - path = repo_root / filename - if path.exists(): - content = path.read_text(encoding="utf-8") - if _CLAUDE_MD_SECTION_MARKER not in content: - targets.append(f"{filename} (append)") - else: - targets.append(f"{filename} (new)") + section = ( + _COPILOT_SECTION + if filename in _PLATFORM_INSTRUCTION_CUSTOM_SECTIONS + else _CLAUDE_MD_SECTION + ) + _describe(filename, repo_root / filename, section) return targets @@ -321,8 +334,7 @@ def _handle_init(args: argparse.Namespace) -> None: from .skills import ( PLATFORMS, generate_skills, - inject_claude_md, - inject_platform_instructions, + inject_instruction_files, install_codebuddy_hooks, install_codebuddy_skills, install_codex_hooks, @@ -364,14 +376,24 @@ def _handle_init(args: argparse.Namespace) -> None: "Inject graph instructions into the files above?", default_yes=True, ): - if target in ("claude", "all"): - inject_claude_md(repo_root) - inject_platform_instructions(repo_root, target=target) - # Use the precomputed instr_targets list for the confirmation - # message; we don't need the fresh return value from - # inject_platform_instructions here. - names = [t.split(" ")[0] for t in instr_targets] - print(f"Injected graph instructions into: {', '.join(names)}") + outcomes = inject_instruction_files(repo_root, target=target) + for label, wording in ( + ("created", "Injected graph instructions into"), + ("updated", "Updated graph instructions in"), + ): + names = [f for f, o in outcomes.items() if o == label] + if names: + print(f"{wording}: {', '.join(names)}") + # A hand-edited block is never overwritten, so say which file it is + # rather than reporting success the user did not get (#314). + stale = [f for f, o in outcomes.items() if o == "conflict"] + if stale: + print( + "Left edited graph instructions alone in: " + f"{', '.join(stale)}. Delete the section between " + " and its closing marker " + "and reinstall to pick up the current text." + ) else: print("Skipped instruction injection (user declined).") elif skip_instructions: diff --git a/code_review_graph/skills.py b/code_review_graph/skills.py index 8f2c15554..0178f12e1 100644 --- a/code_review_graph/skills.py +++ b/code_review_graph/skills.py @@ -19,6 +19,8 @@ from pathlib import Path from typing import Any +from ._legacy_instructions import LEGACY_INSTRUCTION_SECTIONS + logger = logging.getLogger(__name__) @@ -1340,6 +1342,11 @@ def install_codex_hooks(repo_root: Path) -> Path: _CLAUDE_MD_SECTION_MARKER = "" +# Closes the managed block so reinstall can replace it without guessing where it +# ends. Releases before this shipped only the opening marker; those blocks are +# matched by their full text instead, see _legacy_instructions. +_CLAUDE_MD_SECTION_END_MARKER = "" + # Shared across every platform instruction file so the wording stays identical. _INSTRUCTION_INTRO = """**This project has a knowledge graph. Start with the code-review-graph MCP tools to narrow scope, then read the source.** The graph is cheaper than scanning files and @@ -1389,6 +1396,7 @@ def install_codex_hooks(repo_root: Path) -> Path: 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. +{_CLAUDE_MD_SECTION_END_MARKER} """ # Copilot-specific instruction file content: uses VS Code tool references and @@ -1434,6 +1442,7 @@ def install_codex_hooks(repo_root: Path) -> Path: 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. +{_CLAUDE_MD_SECTION_END_MARKER} """ # Maps instruction file path → (marker, section) for files that need content @@ -1448,33 +1457,96 @@ def install_codex_hooks(repo_root: Path) -> Path: } -def _inject_instructions(file_path: Path, marker: str, section: str) -> bool: - """Append an instruction section to a file if not already present. +def _known_instruction_sections() -> tuple[str, ...]: + """Every block text this project has ever generated, longest first. + + Longest first matters: a shorter variant that happens to be contained in a + longer one must never win the match and leave the tail behind. + """ + current = (_CLAUDE_MD_SECTION, _COPILOT_SECTION) + return tuple(sorted({*current, *LEGACY_INSTRUCTION_SECTIONS}, key=len, reverse=True)) + + +def _upgrade_managed_block(existing: str, section: str) -> str | None: + """Replace a previously generated block with ``section``. - Idempotent: checks if the marker is already present before appending. - Creates the file if it doesn't exist. + Only text that exactly equals a known generated block is ever rewritten, so + anything the user wrote around it survives byte for byte. Blocks predating + the end marker have no closing boundary, which is why nothing here searches + for one; guessing where such a block stops would eat user content. - Returns True if the file was modified. + Returns the new file content, or None when the marker is present but no + known block is, meaning someone edited the block by hand. + """ + stale = [ + block + for block in _known_instruction_sections() + if block != section and block in existing + ] + if not stale: + return None + # Anchor on the longest match, then drop any duplicate blocks an older + # release left behind. Splitting around the anchor keeps the cleanup away + # from the text being written in, which a plain str.replace would not. + head = stale[0] + index = existing.index(head) + before, after = existing[:index], existing[index + len(head) :] + for block in stale[1:]: + before = before.replace(block, "") + after = after.replace(block, "") + if section in before or section in after: + # The current block is already there; the stale ones were duplicates. + return before + after + return before + section + after + + +def _inject_instructions(file_path: Path, marker: str, section: str) -> str: + """Create, or upgrade in place, the managed instruction block in a file. + + Returns one of: + + - ``"created"``: the block was written for the first time, creating the + file or appending to one that had no block. + - ``"updated"``: an older generated block was replaced with the current one. + - ``"unchanged"``: the file already holds the current block, byte for byte. + Nothing is written, so repeated installs do not touch the file. + - ``"conflict"``: the marker is present but the block matches nothing this + project generated, so it was hand-edited. The file is left alone and the + caller is expected to tell the user about it. """ existing = "" if file_path.exists(): existing = file_path.read_text(encoding="utf-8", errors="replace") if marker in existing: - logger.info("%s already contains instructions, skipping.", file_path.name) - return False + upgraded = _upgrade_managed_block(existing, section) + if upgraded is None: + if section in existing: + logger.info("%s already holds the current instructions.", file_path.name) + return "unchanged" + logger.warning( + "%s has a hand-edited code-review-graph section; leaving it alone.", + file_path, + ) + return "conflict" + file_path.write_text(upgraded, encoding="utf-8") + logger.info("Updated the MCP tools section in %s", file_path) + return "updated" separator = "\n" if existing and not existing.endswith("\n") else "" extra_newline = "\n" if existing else "" file_path.parent.mkdir(parents=True, exist_ok=True) file_path.write_text(existing + separator + extra_newline + section, encoding="utf-8") logger.info("Appended MCP tools section to %s", file_path) - return True + return "created" + +def inject_claude_md(repo_root: Path) -> str: + """Create or upgrade the MCP tools section in CLAUDE.md. -def inject_claude_md(repo_root: Path) -> None: - """Append MCP tools section to CLAUDE.md.""" - _inject_instructions( + Returns the outcome string documented on ``_inject_instructions``. + """ + return _inject_instructions( repo_root / "CLAUDE.md", _CLAUDE_MD_SECTION_MARKER, _CLAUDE_MD_SECTION, @@ -1509,7 +1581,9 @@ def _remove_legacy_instruction_file(path: Path) -> None: content = path.read_text(encoding="utf-8", errors="replace") if _CLAUDE_MD_SECTION_MARKER not in content: return - for section in (_COPILOT_SECTION, _CLAUDE_MD_SECTION): + # Longest first, so removing a long block cannot leave the tail of a shorter + # variant it contains. Anything not generated by this project is left alone. + for section in _known_instruction_sections(): content = content.replace(section, "") if _CLAUDE_MD_SECTION_MARKER in content: return @@ -1716,9 +1790,30 @@ def inject_platform_instructions(repo_root: Path, target: str = "all") -> list[s - any other platform key (``cursor``, ``windsurf``, ``antigravity``, ``opencode``, ``codex``): writes only the files associated with that platform. - Returns list of filenames that were created or updated. + Returns list of filenames that were created or updated. Use + ``inject_instruction_files`` when the caller also needs to know which files + were left alone because someone edited the block by hand. + """ + outcomes = inject_instruction_files(repo_root, target=target, include_claude_md=False) + return [name for name, outcome in outcomes.items() if outcome in ("created", "updated")] + + +def inject_instruction_files( + repo_root: Path, + target: str = "all", + *, + include_claude_md: bool = True, +) -> dict[str, str]: + """Write every instruction file for ``target`` and report what happened. + + Maps each filename to ``"created"``, ``"updated"``, ``"unchanged"`` or + ``"conflict"``, as documented on ``_inject_instructions``. This is the entry + point the install command uses so it can tell the user which files it + upgraded and which ones need manual attention. """ - updated: list[str] = [] + outcomes: dict[str, str] = {} + if include_claude_md and target in ("claude", "all"): + outcomes["CLAUDE.md"] = inject_claude_md(repo_root) for filename, owners in _PLATFORM_INSTRUCTION_FILES.items(): if target != "all" and target not in owners: continue @@ -1727,13 +1822,12 @@ def inject_platform_instructions(repo_root: Path, target: str = "all") -> list[s marker, section = _PLATFORM_INSTRUCTION_CUSTOM_SECTIONS[filename] else: marker, section = _CLAUDE_MD_SECTION_MARKER, _CLAUDE_MD_SECTION - if _inject_instructions(path, marker, section): - updated.append(filename) + outcomes[filename] = _inject_instructions(path, marker, section) for filename, owners in _LEGACY_PLATFORM_INSTRUCTION_FILES.items(): if target != "all" and target not in owners: continue _remove_legacy_instruction_file(repo_root / filename) - return updated + return outcomes # --- Cursor hooks --- diff --git a/pyproject.toml b/pyproject.toml index 87995c9d6..f2bb21c8c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -114,6 +114,7 @@ select = ["E", "F", "I", "N", "W"] [tool.ruff.lint.per-file-ignores] "code_review_graph/visualization.py" = ["E501"] # embedded HTML/JS template +"code_review_graph/_legacy_instructions.py" = ["E501"] # verbatim past output, must not be reflowed "tests/fixtures/sample_databricks_export.py" = ["F841", "W292"] # intentional fixture patterns "tests/fixtures/sample_notebook.ipynb" = ["F401", "I001"] # fixture imports: intentionally unused, split across cells "tests/test_multilang.py" = ["E501"] # long assertions with explanatory comments diff --git a/tests/test_cli_install.py b/tests/test_cli_install.py index 9ac3b5b2d..0e4fec04a 100644 --- a/tests/test_cli_install.py +++ b/tests/test_cli_install.py @@ -217,9 +217,9 @@ def _install_codebuddy_hooks(repo_root): called["codebuddy_hooks"] = True return repo_root / ".codebuddy" / "settings.json" - def _inject_platform_instructions(repo_root, target="all"): + def _inject_instruction_files(repo_root, target="all", *, include_claude_md=True): called["codebuddy_instructions"] = target == "codebuddy" - return ["CODEBUDDY.md"] + return {"CODEBUDDY.md": "created"} monkeypatch.setattr(skills_module, "generate_skills", _generate_skills) monkeypatch.setattr( @@ -236,8 +236,8 @@ def _inject_platform_instructions(repo_root, target="all"): ) monkeypatch.setattr( skills_module, - "inject_platform_instructions", - _inject_platform_instructions, + "inject_instruction_files", + _inject_instruction_files, ) args = _args(tmp_path, "codebuddy") diff --git a/tests/test_skills.py b/tests/test_skills.py index b2c0ed748..de0d70b6a 100644 --- a/tests/test_skills.py +++ b/tests/test_skills.py @@ -709,6 +709,158 @@ def test_idempotent_with_existing_content(self, tmp_path): assert second_content.count(_CLAUDE_MD_SECTION_MARKER) == 1 +def _legacy_sections(*, copilot: bool) -> list[str]: + """Recorded past blocks, longest first. Copilot ones carry YAML front matter.""" + return [ + block + for block in skills_module.LEGACY_INSTRUCTION_SECTIONS + if block.startswith("---\n") is copilot + ] + + +class TestManagedBlockUpgrade: + """Reinstall must replace an older generated block, not silently skip it. + + Regression test for #314: the injector only checked whether the opening + marker was present, so anyone who installed before the guardrails landed + kept the old text forever and reinstalling was a no-op. + """ + + OLDER = _legacy_sections(copilot=False)[0] + + def test_reinstall_upgrades_an_older_generated_section(self, tmp_path): + claude_md = tmp_path / "CLAUDE.md" + claude_md.write_text(self.OLDER, encoding="utf-8") + + assert inject_claude_md(tmp_path) == "updated" + + content = claude_md.read_text(encoding="utf-8") + assert content == skills_module._CLAUDE_MD_SECTION + assert self.OLDER not in content + assert "### Verify in the source" in content + assert content.count(_CLAUDE_MD_SECTION_MARKER) == 1 + + def test_reinstall_over_current_section_is_byte_idempotent(self, tmp_path): + claude_md = tmp_path / "CLAUDE.md" + assert inject_claude_md(tmp_path) == "created" + first = claude_md.read_bytes() + stat_before = claude_md.stat().st_mtime_ns + + assert inject_claude_md(tmp_path) == "unchanged" + + assert claude_md.read_bytes() == first + # "unchanged" must not rewrite the file at all. + assert claude_md.stat().st_mtime_ns == stat_before + + def test_hand_edited_block_is_preserved_and_reported(self, tmp_path): + edited = self.OLDER.replace("### Key Tools", "### Key Tools (our notes)") + claude_md = tmp_path / "CLAUDE.md" + claude_md.write_text(edited, encoding="utf-8") + + assert inject_claude_md(tmp_path) == "conflict" + + assert claude_md.read_text(encoding="utf-8") == edited + + def test_user_content_around_the_block_survives_an_upgrade(self, tmp_path): + head = "# House rules\n\nNever force push.\n\n" + tail = "\n## Deploy notes\n\nRun the migration first.\n" + claude_md = tmp_path / "CLAUDE.md" + claude_md.write_text(head + self.OLDER + tail, encoding="utf-8") + + assert inject_claude_md(tmp_path) == "updated" + + content = claude_md.read_text(encoding="utf-8") + assert content == head + skills_module._CLAUDE_MD_SECTION + tail + assert content.startswith(head) + assert content.endswith(tail) + + def test_duplicate_stale_blocks_collapse_to_one(self, tmp_path): + """#558 left repeat installs stacking blocks; upgrade must not keep both.""" + older_two = _legacy_sections(copilot=False)[1] + claude_md = tmp_path / "CLAUDE.md" + claude_md.write_text(self.OLDER + "\n" + older_two, encoding="utf-8") + + assert inject_claude_md(tmp_path) == "updated" + + content = claude_md.read_text(encoding="utf-8") + assert content.count(_CLAUDE_MD_SECTION_MARKER) == 1 + assert skills_module._CLAUDE_MD_SECTION in content + + def test_missing_file_is_still_created(self, tmp_path): + assert not (tmp_path / "CLAUDE.md").exists() + + assert inject_claude_md(tmp_path) == "created" + + assert (tmp_path / "CLAUDE.md").read_text(encoding="utf-8") == ( + skills_module._CLAUDE_MD_SECTION + ) + + def test_new_sections_carry_an_end_marker(self, tmp_path): + end = skills_module._CLAUDE_MD_SECTION_END_MARKER + inject_claude_md(tmp_path) + skills_module.inject_platform_instructions(tmp_path, target="all") + + names = ["CLAUDE.md", *skills_module._PLATFORM_INSTRUCTION_FILES] + for name in names: + content = (tmp_path / name).read_text(encoding="utf-8") + assert content.count(end) == 1, name + assert content.index(_CLAUDE_MD_SECTION_MARKER) < content.index(end), name + + def test_every_platform_file_upgrades_from_its_older_section(self, tmp_path): + older_copilot = _legacy_sections(copilot=True)[0] + for name in skills_module._PLATFORM_INSTRUCTION_FILES: + path = tmp_path / name + path.parent.mkdir(parents=True, exist_ok=True) + custom = name in skills_module._PLATFORM_INSTRUCTION_CUSTOM_SECTIONS + path.write_text(older_copilot if custom else self.OLDER, encoding="utf-8") + + outcomes = skills_module.inject_instruction_files(tmp_path, target="all") + + for name in skills_module._PLATFORM_INSTRUCTION_FILES: + assert outcomes[name] == "updated", name + content = (tmp_path / name).read_text(encoding="utf-8") + assert "### Verify in the source" in content, name + assert "ALWAYS use the" not in content, name + + def test_legacy_sections_are_exact_and_ordered_longest_first(self): + legacy = skills_module.LEGACY_INSTRUCTION_SECTIONS + assert len(set(legacy)) == len(legacy) + assert all(_CLAUDE_MD_SECTION_MARKER in block for block in legacy) + known = skills_module._known_instruction_sections() + assert list(known) == sorted(known, key=len, reverse=True) + assert skills_module._CLAUDE_MD_SECTION in known + assert skills_module._COPILOT_SECTION in known + + +class TestInjectInstructionFilesOutcomes: + def test_reports_created_then_unchanged(self, tmp_path): + first = skills_module.inject_instruction_files(tmp_path, target="all") + assert set(first) == {"CLAUDE.md", *skills_module._PLATFORM_INSTRUCTION_FILES} + assert set(first.values()) == {"created"} + + second = skills_module.inject_instruction_files(tmp_path, target="all") + assert set(second.values()) == {"unchanged"} + + def test_reports_conflict_without_touching_the_file(self, tmp_path): + edited = _CLAUDE_MD_SECTION_MARKER + "\n## Our own rules\n" + (tmp_path / "CLAUDE.md").write_text(edited, encoding="utf-8") + + outcomes = skills_module.inject_instruction_files(tmp_path, target="claude") + + assert outcomes == {"CLAUDE.md": "conflict"} + assert (tmp_path / "CLAUDE.md").read_text(encoding="utf-8") == edited + + def test_platform_wrapper_still_returns_written_filenames(self, tmp_path): + first = inject_platform_instructions(tmp_path, target="windsurf") + assert first == [".windsurfrules"] + assert inject_platform_instructions(tmp_path, target="windsurf") == [] + + (tmp_path / ".windsurfrules").write_text( + _legacy_sections(copilot=False)[0], encoding="utf-8" + ) + assert inject_platform_instructions(tmp_path, target="windsurf") == [".windsurfrules"] + + class TestInjectPlatformInstructionsFiltering: def test_all_writes_every_file(self, tmp_path): updated = inject_platform_instructions(tmp_path, target="all") From 65f08f13fcd98c82046904958040a06a0c87350d Mon Sep 17 00:00:00 2001 From: Tirth Kanani Date: Wed, 19 Aug 2026 20:02:38 +0100 Subject: [PATCH 3/4] docs: regenerate this repo's instruction files through the install path Ran skills.inject_instruction_files against this checkout rather than hand-editing, so the upgrade path from the previous commit is proven on real files that have carried the stale wording since before the guardrails existed. CLAUDE.md, AGENTS.md and GEMINI.md each held the previous generated block with a markdown formatter's normalized table separator; all three upgraded in place and every byte outside the block is unchanged. .github/code-review-graph.instruction.md is a superseded path, so the same run removed it, taking with it a Copilot variant that told agents to prefer the graph over #tool:search. .github/copilot-instructions.md is not owned by the platform table, so it went through skills._inject_instructions directly with the same marker and section. The install path also writes .cursorrules, .windsurfrules, CODEBUDDY.md, QODER.md and the .kiro and .github/instructions files. This repo does not track those, so they were removed again rather than committed. Refs #314 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012fHfGDiZedoxjpKzanHri3 --- .github/code-review-graph.instruction.md | 43 ------------------------ .github/copilot-instructions.md | 19 +++++++---- AGENTS.md | 21 ++++++++---- CLAUDE.md | 21 ++++++++---- GEMINI.md | 21 ++++++++---- 5 files changed, 55 insertions(+), 70 deletions(-) delete mode 100644 .github/code-review-graph.instruction.md diff --git a/.github/code-review-graph.instruction.md b/.github/code-review-graph.instruction.md deleted file mode 100644 index 0cdcabb2e..000000000 --- a/.github/code-review-graph.instruction.md +++ /dev/null @@ -1,43 +0,0 @@ ---- -applyTo: '**' -description: Use code-review-graph MCP tools for token-efficient codebase exploration and code review instead of built-in file/search tools. ---- - - -## MCP Tools: code-review-graph - -**IMPORTANT: This project has a knowledge graph. ALWAYS use the -code-review-graph MCP tools BEFORE using #tool:read/readFile #tool:search/fileSearch #tool:search/textSearch to explore -the codebase.** The graph is faster, cheaper (fewer tokens), and gives -you structural context (callers, dependents, test coverage) that file -scanning cannot. - -### When to use graph tools FIRST - -- **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_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_tool` for code review. -3. Use `get_affected_flows_tool` to understand impact. -4. Use `query_graph_tool` pattern="tests_for" to check coverage. diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 0a9468fc6..b8f24fb6c 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,11 +1,9 @@ ## MCP Tools: code-review-graph -**IMPORTANT: This project has a knowledge graph. ALWAYS use the -code-review-graph MCP tools BEFORE using Grep/Glob/Read to explore -the codebase.** The graph is faster, cheaper (fewer tokens), and gives -you structural context (callers, dependents, test coverage) that file -scanning cannot. +**This project has a knowledge graph. Start with the code-review-graph +MCP tools to narrow scope, then read the source.** The graph is cheaper than scanning files and +gives you structural context (callers, dependents, test coverage) that file search cannot. ### When to use graph tools FIRST @@ -15,7 +13,15 @@ scanning cannot. - **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. +### Verify in the source + +- Narrow scope with the graph, then read the source. Do not change code from graph output alone. +- For any non-trivial change, read the implementation and the relevant tests before concluding. +- Verify the exact source when touching behavior, database logic, migrations, retries, fallbacks, + recovery, or compatibility code. +- When the graph and the source disagree, the source wins. The graph may be stale or may not + model that relationship. +- An empty graph result can mean "not indexed" or "not statically visible", not "does not exist". ### Key Tools @@ -36,3 +42,4 @@ Fall back to Grep/Glob/Read **only** when the graph doesn't cover what you need. 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. + diff --git a/AGENTS.md b/AGENTS.md index 418ea2c2c..96d937173 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -86,11 +86,9 @@ bd close # Complete work ## MCP Tools: code-review-graph -**IMPORTANT: This project has a knowledge graph. ALWAYS use the -code-review-graph MCP tools BEFORE using Grep/Glob/Read to explore -the codebase.** The graph is faster, cheaper (fewer tokens), and gives -you structural context (callers, dependents, test coverage) that file -scanning cannot. +**This project has a knowledge graph. Start with the code-review-graph +MCP tools to narrow scope, then read the source.** The graph is cheaper than scanning files and +gives you structural context (callers, dependents, test coverage) that file search cannot. ### When to use graph tools FIRST @@ -100,12 +98,20 @@ scanning cannot. - **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. +### Verify in the source + +- Narrow scope with the graph, then read the source. Do not change code from graph output alone. +- For any non-trivial change, read the implementation and the relevant tests before concluding. +- Verify the exact source when touching behavior, database logic, migrations, retries, fallbacks, + recovery, or compatibility code. +- When the graph and the source disagree, the source wins. The graph may be stale or may not + model that relationship. +- An empty graph result can mean "not indexed" or "not statically visible", not "does not exist". ### Key Tools | Tool | Use when | -|------|----------| +| ------ | ---------- | | `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 | @@ -121,3 +127,4 @@ Fall back to Grep/Glob/Read **only** when the graph doesn't cover what you need. 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. + diff --git a/CLAUDE.md b/CLAUDE.md index 47727eb1a..df2527a9a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -169,11 +169,9 @@ bd close # Complete work ## MCP Tools: code-review-graph -**IMPORTANT: This project has a knowledge graph. ALWAYS use the -code-review-graph MCP tools BEFORE using Grep/Glob/Read to explore -the codebase.** The graph is faster, cheaper (fewer tokens), and gives -you structural context (callers, dependents, test coverage) that file -scanning cannot. +**This project has a knowledge graph. Start with the code-review-graph +MCP tools to narrow scope, then read the source.** The graph is cheaper than scanning files and +gives you structural context (callers, dependents, test coverage) that file search cannot. ### When to use graph tools FIRST @@ -183,12 +181,20 @@ scanning cannot. - **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. +### Verify in the source + +- Narrow scope with the graph, then read the source. Do not change code from graph output alone. +- For any non-trivial change, read the implementation and the relevant tests before concluding. +- Verify the exact source when touching behavior, database logic, migrations, retries, fallbacks, + recovery, or compatibility code. +- When the graph and the source disagree, the source wins. The graph may be stale or may not + model that relationship. +- An empty graph result can mean "not indexed" or "not statically visible", not "does not exist". ### Key Tools | Tool | Use when | -|------|----------| +| ------ | ---------- | | `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 | @@ -204,3 +210,4 @@ Fall back to Grep/Glob/Read **only** when the graph doesn't cover what you need. 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. + diff --git a/GEMINI.md b/GEMINI.md index 3d7c6cfa1..b8f24fb6c 100644 --- a/GEMINI.md +++ b/GEMINI.md @@ -1,11 +1,9 @@ ## MCP Tools: code-review-graph -**IMPORTANT: This project has a knowledge graph. ALWAYS use the -code-review-graph MCP tools BEFORE using Grep/Glob/Read to explore -the codebase.** The graph is faster, cheaper (fewer tokens), and gives -you structural context (callers, dependents, test coverage) that file -scanning cannot. +**This project has a knowledge graph. Start with the code-review-graph +MCP tools to narrow scope, then read the source.** The graph is cheaper than scanning files and +gives you structural context (callers, dependents, test coverage) that file search cannot. ### When to use graph tools FIRST @@ -15,12 +13,20 @@ scanning cannot. - **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. +### Verify in the source + +- Narrow scope with the graph, then read the source. Do not change code from graph output alone. +- For any non-trivial change, read the implementation and the relevant tests before concluding. +- Verify the exact source when touching behavior, database logic, migrations, retries, fallbacks, + recovery, or compatibility code. +- When the graph and the source disagree, the source wins. The graph may be stale or may not + model that relationship. +- An empty graph result can mean "not indexed" or "not statically visible", not "does not exist". ### Key Tools | Tool | Use when | -|------|----------| +| ------ | ---------- | | `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 | @@ -36,3 +42,4 @@ Fall back to Grep/Glob/Read **only** when the graph doesn't cover what you need. 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. + From 355d2b59106efb1011ff3528a6db9c99f78ee29c Mon Sep 17 00:00:00 2001 From: Tirth Kanani Date: Wed, 19 Aug 2026 20:10:00 +0100 Subject: [PATCH 4/4] fix(uninstall): remove an instruction block written by any past release _remove_instruction matched only the section text the running version generates. Install can now upgrade a block from any release, so leaving removal on a single exact text meant a user who asked for the instructions to go kept whichever wording their original install wrote, reported as "differs from a known installed section" and silently left in place. Removal now walks skills._known_instruction_sections(), the same recorded texts install matches, longest first so a short variant cannot strand the tail of a longer one that contains it. That covers every past release including the variant a markdown formatter rewrote in this repo's own files, and clears duplicate blocks an older release stacked up. The safety rules are unchanged: exact match only, no end boundary is ever guessed for a block that has no closing marker, and a section that matches nothing known is left alone and reported rather than deleted. A file holding both a generated and a hand-edited block loses only the generated one and is named in the report. _join_without_instruction closes the gap. Text on both sides is kept and only the whitespace at the seam is normalised, so removing a block from the middle of a file leaves one blank line rather than a pile of them, and removing one appended at the end restores the file byte for byte. Refs #314, #558 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012fHfGDiZedoxjpKzanHri3 --- CHANGELOG.md | 6 ++ code_review_graph/uninstall.py | 88 +++++++++++++++------- tests/test_uninstall.py | 133 +++++++++++++++++++++++++++++++++ 3 files changed, 198 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4fd6a1eaf..42abe9a0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,12 @@ by hand is left alone and named in the install output instead of being overwritten, and `install` now reports created, updated and left-alone files separately (#314, #558). +- `uninstall` now removes an instruction section written by any past release, + not only one written by the running version, so asking for the instructions + to go no longer leaves orphaned text behind. It matches the same recorded + block texts install uses, clears duplicate blocks, keeps the text on both + sides of the block, closes the gap without leaving a run of blank lines, and + still refuses to touch a section that was edited by hand (#314). - C# receiver calls (`Service.StaticCall()`, `obj.Method()`, `obj?.Method()`) now resolve to canonical method nodes using receiver-type and namespace evidence recorded at parse time, so `callers_of`, `get_impact_radius`, and diff --git a/code_review_graph/uninstall.py b/code_review_graph/uninstall.py index ea12b31e4..6aabdd4f2 100644 --- a/code_review_graph/uninstall.py +++ b/code_review_graph/uninstall.py @@ -839,34 +839,69 @@ def _remove_skill_file( _prune_empty_directory(path.parent, boundary) +def _join_without_instruction(before: str, after: str) -> str: + """Close the gap a removed instruction block leaves behind. + + The text on either side is kept. Only the whitespace at the seam is + normalised, so removing a block from the middle of a file does not leave a + pile of blank lines where it used to be. + """ + if not after.strip(): + # The block ran to the end, which is where install appends it. + return before.rstrip() + ("\n" if before.strip() else "") + if not before.strip(): + return after.lstrip("\n") + return before.rstrip("\n") + "\n\n" + after.lstrip("\n") + + def _remove_instruction( path: Path, - section: str, boundary: Path, report: UninstallReport, *, dry_run: bool, ) -> None: + """Strip every generated instruction block from a file. + + Only text that exactly equals a block this project generated is removed, so + a section someone edited by hand survives and is reported instead. Blocks + written before the closing marker existed have no end boundary, which is why + nothing here searches for one: their full recorded text is the boundary. + Matching every known variant means a block from any past release comes out, + not just one written by the running version (#314). + """ if not path.exists() or not _safe_path(path, boundary, report): return raw = _read_text(path, report) if raw is None: return - exact_index = raw.find(section) - if exact_index >= 0: - start = exact_index - end = exact_index + len(section) - rewritten = raw[:start] + raw[end:] - else: - marker_index = raw.find(skills._CLAUDE_MD_SECTION_MARKER) - if marker_index < 0: - return + + rewritten = raw + # Longest first, so removing a short variant cannot strand the tail of a + # longer one that contains it. The loop also clears duplicate blocks that + # older releases stacked up (#558). + for known in skills._known_instruction_sections(): + while (index := rewritten.find(known)) >= 0: + rewritten = _join_without_instruction( + rewritten[:index], rewritten[index + len(known) :] + ) + + if rewritten == raw: + if skills._CLAUDE_MD_SECTION_MARKER in raw: + report.skipped_paths.append( + f"{path} (marked instruction section differs from a known installed section; " + "left unchanged)" + ) + return + + if skills._CLAUDE_MD_SECTION_MARKER in rewritten: + # One block was generated and another was edited. Remove what this + # project owns and name the file so the user can deal with the rest. report.skipped_paths.append( - f"{path} (marked instruction section differs from a known installed section; " - "left unchanged)" + f"{path} (a further marked instruction section differs from a known " + "installed section; left unchanged)" ) - return - rewritten = rewritten.rstrip() + ("\n" if rewritten.strip() else "") + if rewritten: _write_text( path, @@ -1211,23 +1246,18 @@ def _process_repo( dry_run=dry_run, ) - instruction_sections = { - "CLAUDE.md": skills._CLAUDE_MD_SECTION, - **{ - relative: skills._PLATFORM_INSTRUCTION_CUSTOM_SECTIONS.get( - relative, - (skills._CLAUDE_MD_SECTION_MARKER, skills._CLAUDE_MD_SECTION), - )[1] - for relative in ( - *skills._PLATFORM_INSTRUCTION_FILES, - *skills._LEGACY_PLATFORM_INSTRUCTION_FILES, - ) - }, - } - for relative, section in instruction_sections.items(): + # Every variant is matched per file, so which section a given path was + # written with no longer has to be worked out here. + instruction_files = dict.fromkeys( + ( + "CLAUDE.md", + *skills._PLATFORM_INSTRUCTION_FILES, + *skills._LEGACY_PLATFORM_INSTRUCTION_FILES, + ) + ) + for relative in instruction_files: _remove_instruction( repo_root / relative, - section, repo_root, report, dry_run=dry_run, diff --git a/tests/test_uninstall.py b/tests/test_uninstall.py index 86c460211..f02b8e399 100644 --- a/tests/test_uninstall.py +++ b/tests/test_uninstall.py @@ -457,6 +457,139 @@ def test_uninstall_cleans_current_and_legacy_copilot_instruction_paths( assert path.read_text(encoding="utf-8") == "# User notes\n" +def _legacy_claude_section() -> str: + """The longest recorded pre-guardrails CLAUDE.md-shaped block.""" + return next( + block + for block in skills.LEGACY_INSTRUCTION_SECTIONS + if not block.startswith("---\n") + ) + + +def test_uninstall_removes_a_section_written_by_an_older_release( + fake_repo: Path, + fake_home: Path, +) -> None: + """Removal used to match only the running version's text, so a block from + any earlier release survived an explicit uninstall (#314).""" + path = fake_repo / "CLAUDE.md" + _write(path, "user instructions\n\n" + _legacy_claude_section()) + + report = uninstall.run(repo=fake_repo, keep_data=True) + + assert report.errors == [] + assert path.read_text(encoding="utf-8") == "user instructions\n" + assert report.skipped_paths == [] + + +def test_uninstall_removes_the_current_section_including_its_end_marker( + fake_repo: Path, + fake_home: Path, +) -> None: + path = fake_repo / "CLAUDE.md" + _write(path, "user instructions\n\n" + skills._CLAUDE_MD_SECTION) + + uninstall.run(repo=fake_repo, keep_data=True) + + remaining = path.read_text(encoding="utf-8") + assert remaining == "user instructions\n" + assert skills._CLAUDE_MD_SECTION_END_MARKER not in remaining + + +def test_uninstall_leaves_a_hand_edited_section_alone_and_reports_it( + fake_repo: Path, + fake_home: Path, +) -> None: + path = fake_repo / "CLAUDE.md" + content = "user prefix\n\n" + _legacy_claude_section().replace( + "### Key Tools", "### Key Tools (our notes)" + ) + _write(path, content) + + report = uninstall.run(repo=fake_repo, keep_data=True) + + assert path.read_text(encoding="utf-8") == content + assert any(str(path) in item and "left unchanged" in item for item in report.skipped_paths) + + +def test_uninstall_keeps_user_content_on_both_sides_of_the_block( + fake_repo: Path, + fake_home: Path, +) -> None: + head = "# House rules\n\nNever force push.\n\n" + tail = "\n## Deploy notes\n\nRun the migration first.\n" + path = fake_repo / "CLAUDE.md" + _write(path, head + _legacy_claude_section() + tail) + + uninstall.run(repo=fake_repo, keep_data=True) + + remaining = path.read_text(encoding="utf-8") + assert remaining == ( + "# House rules\n\nNever force push.\n\n## Deploy notes\n\nRun the migration first.\n" + ) + assert skills._CLAUDE_MD_SECTION_MARKER not in remaining + # One blank line where the block was, not a pileup left by the removal. + assert "\n\n\n" not in remaining + + +def test_uninstall_clears_every_duplicate_stale_block( + fake_repo: Path, + fake_home: Path, +) -> None: + """Repeat installs used to stack blocks (#558); removal must clear them all.""" + older = [ + block + for block in skills.LEGACY_INSTRUCTION_SECTIONS + if not block.startswith("---\n") + ] + path = fake_repo / "CLAUDE.md" + _write(path, "user instructions\n\n" + older[0] + "\n" + older[1] + "\n" + older[0]) + + report = uninstall.run(repo=fake_repo, keep_data=True) + + assert report.errors == [] + assert path.read_text(encoding="utf-8") == "user instructions\n" + + +def test_install_then_uninstall_restores_the_file_byte_for_byte( + fake_repo: Path, + fake_home: Path, +) -> None: + original = "# House rules\n\nNever force push.\n" + path = fake_repo / "CLAUDE.md" + _write(path, original) + + skills.inject_claude_md(fake_repo) + assert path.read_text(encoding="utf-8") != original + + uninstall.run(repo=fake_repo, keep_data=True) + + assert path.read_text(encoding="utf-8") == original + + +def test_uninstall_removes_older_sections_from_every_instruction_path( + fake_repo: Path, + fake_home: Path, +) -> None: + older_claude = _legacy_claude_section() + older_copilot = next( + block for block in skills.LEGACY_INSTRUCTION_SECTIONS if block.startswith("---\n") + ) + relatives = ["CLAUDE.md", *skills._PLATFORM_INSTRUCTION_FILES] + for relative in relatives: + custom = relative in skills._PLATFORM_INSTRUCTION_CUSTOM_SECTIONS + _write( + fake_repo / relative, + "user instructions\n\n" + (older_copilot if custom else older_claude), + ) + + report = uninstall.run(repo=fake_repo, keep_data=True) + + assert report.errors == [] + for relative in relatives: + assert (fake_repo / relative).read_text(encoding="utf-8") == "user instructions\n" + + def test_modified_instruction_section_is_not_guessed_or_truncated( fake_repo: Path, fake_home: Path,