fix(instructions): graph narrows scope, source verifies, and upgrades actually reach existing users - #886
Merged
Conversation
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012fHfGDiZedoxjpKzanHri3
…he 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 <!-- /code-review-graph MCP tools --> 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012fHfGDiZedoxjpKzanHri3
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012fHfGDiZedoxjpKzanHri3
_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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012fHfGDiZedoxjpKzanHri3
…-and-platform-policy
code-review-graph reviewOverall risk: 0.65 (MEDIUM) — 45 changed function(s)/class(es), 0 affected flow(s), 12 test gap(s) Risk-scored changes
Test gaps
Token savings: this graph-backed report used ~120,364 fewer tokens (~95%) than reading every changed file in full (estimated, chars/4 approximation). Powered by code-review-graph — local-first analysis; no code leaves the CI runner. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #314 and #883, and adds a contribution bar for new platform targets.
1. The instruction text told agents to trust the graph over the source (#314)
The injected section said 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 models acted on graph summaries, skipped the implementation, and produced shallower work. Several commenters on #314 describe exactly that.
The absolute wording is gone. The section now says to start with the graph to narrow scope, then read the source, and carries six guardrails: do not change code from graph output alone; read the implementation and its tests before a non-trivial change; verify the exact source for behaviour, database, migration, retry, fallback, recovery and compatibility code; when the graph and the source disagree, the source wins; and an empty result can mean "not indexed" or "not statically visible", not "does not exist".
That last line is the instruction-layer half of #884 and #885, which say the same thing at the tool-output layer.
Because this text costs every user tokens in every session, the old intro was tightened to pay for most of the space.
_CLAUDE_MD_SECTIONgoes from 38 to 45 lines,_COPILOT_SECTIONfrom 46 to 52. The intro and guardrails are shared constants, so all nine generated targets stay byte-identical.2. Those improvements would never have reached anyone (#883)
_inject_instructionswas append-only behind a marker guard: if the marker was present it logged "already contains instructions, skipping" and returned without ever comparing content. Once a user had installed, no later release could change that section. Every improvement above would have reached only brand-new installs.Generated sections now carry a closing marker so they have real boundaries. Older blocks have none, so
_upgrade_managed_blockmatches their full recorded text, longest first, and never guesses where a boundary-less block ends, because guessing is what would eat user content.code_review_graph/_legacy_instructions.pyrecords the 10 past block texts, recovered by walking the git history ofskills.pyand by scanning the instruction files this repo ships. That second pass mattered: this repo's own CLAUDE.md, AGENTS.md and GEMINI.md carried a block a markdown formatter had reflowed, which the source history alone would never have produced, and those three files would otherwise have been reported as hand-edited._inject_instructionsnow reportscreated,updated,unchangedorconflict. A hand-edited block is left exactly as the user wrote it and named in the output. Reinstalling an unchanged file does not even touch its mtime.uninstallwas the same bug in reverse, refusing to remove a block written by an older release, and now consults the same recorded list.What an existing user sees on their next install:
This repository's own instruction files were regenerated through that path rather than hand-edited, so the upgrade is proven end to end. Every byte outside the managed block is identical to HEAD.
3. A bar for new platform targets
CONTRIBUTING.md gains an "Adding a Platform Target" section. Every supported tool is permanent maintenance surface, and the ones that break are the ones merged without evidence they ever worked in a released client. New targets need official MCP config documentation, the exact path and schema, an entry through the existing PLATFORMS table, preserved unrelated settings, byte-idempotent reinstall, a working uninstall, lifecycle tests matching the existing ones, and a screenshot or transcript from a real released client showing a CRG tool being invoked. A rendered image of text does not count. Requests no maintainer can install and test may be declined or left open.
Tests
24 new: 5 guardrail assertions across all nine targets, 12 for the install upgrade path (upgrade from each recorded variant, byte-idempotent reinstall, hand-edited block preserved and reported, user content around the block survives, duplicate stale blocks collapsed), and 7 for uninstall including a full install-then-uninstall byte-for-byte round trip.
Full suite 2783 passed, 5 skipped, 2 xpassed. ruff and mypy clean.