Skip to content

Commit fef54f7

Browse files
nhortonclaude
andcommitted
feat: inject sorted git diff into broad review rule prompts
Rules with strategy all_changed_files or matches_together and a **/* matcher now get git diff main..HEAD pre-fetched and injected into the review instruction file. The diff is sorted by filepath to group files by directory, reducing reviewer turn count. Diff is scoped to the rule's source_dir so subdirectory .deepreview files get narrower diffs. Also streamlines the /review skill to skip the get_configured_reviews call and go straight to get_review_instructions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent c4ac229 commit fef54f7

9 files changed

Lines changed: 457 additions & 9 deletions

File tree

plugins/claude/skills/review/SKILL.md

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,16 +18,12 @@ Only proceed past this section if the user wants to **run** reviews.
1818

1919
## How to Run
2020

21-
1. First, call `mcp__deepwork__get_configured_reviews` to see what review rules are configured. This returns each rule's name, description, and which `.deepreview` file defines it. If reviewing specific files, pass `only_rules_matching_files` to see only the rules that apply. Share a brief summary of the active rules with the user before proceeding.
22-
- **If no rules are configured**:
23-
1. Use AskUserQuestion to tell the user there are no `.deepreview` rules set up yet and ask if they'd like the agent to auto-discover and suggest rules for this project.
24-
2. If yes, invoke the `/deepwork` skill with the `deepwork_reviews` job's `discover_rules` workflow (which sets up native reviews, skill migration, documentation rules, and language-specific code review).
25-
3. Stop here — do not proceed with running reviews if there are no rules.
26-
2. Call the `mcp__deepwork__get_review_instructions` tool:
21+
1. Call the `mcp__deepwork__get_review_instructions` tool directly:
2722
- **No arguments** to review the current branch's changes (auto-detects via git diff against the main branch).
2823
- **With `files`** to review only specific files: `mcp__deepwork__get_review_instructions(files=["src/app.py", "src/lib.py"])`. When provided, only reviews whose include/exclude patterns match the given files will be returned. Use this when the user asks to review a particular file or set of files rather than the whole branch.
29-
3. The output will list review tasks to invoke in parallel. Each task has `name`, `description`, `subagent_type`, and `prompt` fields — these map directly to the Task tool parameters. Launch all of them as parallel Task agents.
30-
4. Collect the results from all review agents.
24+
- **If the result says no rules are configured**: Ask the user if they'd like to auto-discover and set up rules. If yes, invoke the `/deepwork` skill with the `deepwork_reviews` job's `discover_rules` workflow. Stop here — do not proceed with running reviews if there are no rules.
25+
2. The output will list review tasks to invoke in parallel. Each task has `name`, `description`, `subagent_type`, and `prompt` fields — these map directly to the Task tool parameters. Launch all of them as parallel Task agents.
26+
3. Collect the results from all review agents.
3127

3228
## Acting on Results
3329

specs/deepwork/review/REVIEW-REQ-004-rule-matching-and-strategies.md

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ After discovering review rules (REVIEW-REQ-002) and changed files (REVIEW-REQ-00
1919
### REVIEW-REQ-004.2: Review Task Data Model
2020

2121
1. Each review task MUST be represented as a `ReviewTask` dataclass.
22-
2. The `ReviewTask` MUST contain: `rule_name` (str), `files_to_review` (list[str] — paths relative to repo root), `instructions` (str), `agent_name` (str | None), `source_location` (str — formatted as `"path:line"`), `additional_files` (list[str] — unchanged matching files, relative to repo root), `all_changed_filenames` (list[str] | None).
22+
2. The `ReviewTask` MUST contain: `rule_name` (str), `files_to_review` (list[str] — paths relative to repo root), `instructions` (str), `agent_name` (str | None), `source_location` (str — formatted as `"path:line"`), `additional_files` (list[str] — unchanged matching files, relative to repo root), `all_changed_filenames` (list[str] | None), `git_diff_output` (str | None — pre-fetched diff for broad rules, see REVIEW-REQ-004.11).
2323
3. `files_to_review` MUST always contain at least one file path.
2424
4. `source_location` MUST be formatted as `"{relative_path}:{line_number}"` where the path is relative to the project root (e.g., `"src/.deepreview:5"`).
2525

@@ -71,3 +71,12 @@ After discovering review rules (REVIEW-REQ-002) and changed files (REVIEW-REQ-00
7171
1. Rules with the same name defined in different `.deepreview` files MUST produce independent `ReviewTask` objects. The system MUST NOT merge or combine matched files across rules from different source directories.
7272
2. When two `.deepreview` files in different directories define a rule with the same name and the same strategy, and changed files match both rules, the system MUST create separate `ReviewTask` objects — one per directory — each containing only the files that matched within its own `source_dir`.
7373
3. This isolation is a consequence of REVIEW-REQ-004.1.2 (files outside `source_dir` do not match) but is stated explicitly because `.deepreview` files can be templated or symlinked across directories, making same-name rules a common scenario.
74+
75+
### REVIEW-REQ-004.11: Git Diff Injection for Broad Rules
76+
77+
1. When a rule has strategy `"all_changed_files"` or `"matches_together"` AND its `include` patterns contain `**/*`, the system MUST run `git diff <merge-base>..HEAD` and attach the output to the resulting `ReviewTask` as `git_diff_output`.
78+
2. The git diff MUST be computed at most once per unique `source_dir` per `match_files_to_rules` invocation, even if multiple rules with the same `source_dir` qualify for injection.
79+
3. If the git diff command fails or produces empty output, `git_diff_output` MUST be `None`.
80+
4. Rules with strategy `"individual"` MUST NOT receive `git_diff_output`, regardless of their include patterns.
81+
5. The `ReviewTask` dataclass MUST include a `git_diff_output: str | None` field defaulting to `None`.
82+
6. When a rule's `source_dir` is a subdirectory of the project root, the git diff MUST be scoped to that subdirectory (via `-- <relpath>` pathspec). When `source_dir` equals the project root, the diff MUST cover the entire repository.

specs/deepwork/review/REVIEW-REQ-005-instruction-generation.md

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,13 @@ For each `ReviewTask`, the system generates a self-contained markdown instructio
1616
6. When the task has `additional_files` (unchanged matching files), the file MUST contain an "Unchanged Matching Files" section listing those file paths.
1717
7. When the task has `all_changed_filenames`, the file MUST contain an "All Changed Files" section listing every changed filename for context.
1818

19+
### REVIEW-REQ-005.7: Git Diff Section
20+
21+
1. When a `ReviewTask` has a non-null `git_diff_output`, the instruction file MUST contain a section headed `## Output from \`git diff main..HEAD\` for you to review (sorted by filepath)`.
22+
2. The diff output MUST be rendered inside a fenced code block with the `diff` language tag.
23+
3. This section MUST appear after the "Files to Review" section and before the "All Changed Files" section.
24+
4. When `git_diff_output` is `None`, this section MUST be omitted.
25+
1926
### REVIEW-REQ-005.2: File Path Formatting
2027

2128
1. File paths in the "Files to Review" section MUST be prefixed with `@` to trigger Claude Code's file-reading behavior (e.g., `@src/app.py`).

src/deepwork/review/config.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ class ReviewTask:
4949
source_location: str = "" # e.g. "src/.deepreview:5"
5050
additional_files: list[str] = field(default_factory=list) # Unchanged matching files
5151
all_changed_filenames: list[str] | None = None
52+
git_diff_output: str | None = None # Pre-fetched git diff for broad rules
5253

5354

5455
def parse_deepreview_file(filepath: Path) -> list[ReviewRule]:

src/deepwork/review/instructions.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,14 @@ def build_instruction_file(task: ReviewTask, review_id: str = "") -> str:
155155
parts.append(f"- @{filepath}")
156156
parts.append("")
157157

158+
# Pre-fetched git diff for broad rules
159+
if task.git_diff_output:
160+
parts.append("## Output from `git diff main..HEAD` for you to review (sorted by filepath)\n")
161+
parts.append("```diff")
162+
parts.append(task.git_diff_output.rstrip())
163+
parts.append("```")
164+
parts.append("")
165+
158166
# Additional context: all changed filenames
159167
if task.all_changed_filenames:
160168
parts.append("## All Changed Files\n")

src/deepwork/review/matcher.py

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,75 @@ def _git_untracked_files(project_root: Path) -> list[str]:
192192
raise GitDiffError(f"git ls-files failed: {e.stderr.strip()}") from e
193193

194194

195+
def _should_inject_diff(rule: ReviewRule) -> bool:
196+
"""Check if a rule qualifies for git diff injection.
197+
198+
Rules with ``all_changed_files`` or ``matches_together`` strategy and
199+
a ``**/*`` include pattern get the diff injected to reduce reviewer
200+
turn count.
201+
"""
202+
if rule.strategy not in ("all_changed_files", "matches_together"):
203+
return False
204+
return "**/*" in rule.include_patterns
205+
206+
207+
def _sort_diff_by_path(diff_text: str) -> str:
208+
"""Sort a unified diff's file hunks by path.
209+
210+
Splits on ``diff --git`` boundaries, sorts the chunks
211+
alphabetically by file path (which naturally groups by directory),
212+
and rejoins them.
213+
"""
214+
if not diff_text.strip():
215+
return diff_text
216+
217+
# Split into per-file chunks. The first element before the first
218+
# "diff --git" marker is usually empty or whitespace — preserve it.
219+
parts = re.split(r"(?=^diff --git )", diff_text, flags=re.MULTILINE)
220+
chunks: list[tuple[str, str]] = []
221+
preamble = ""
222+
for part in parts:
223+
if part.startswith("diff --git "):
224+
# Extract the b-side path: "diff --git a/x b/y" → "y"
225+
first_line = part.split("\n", 1)[0]
226+
b_path = first_line.rsplit(" b/", 1)[-1] if " b/" in first_line else first_line
227+
chunks.append((b_path, part))
228+
elif part.strip():
229+
preamble += part
230+
231+
chunks.sort(key=lambda c: c[0])
232+
sorted_parts = [c[1] for c in chunks]
233+
if preamble:
234+
sorted_parts.insert(0, preamble)
235+
return "".join(sorted_parts)
236+
237+
238+
def _get_git_diff(project_root: Path, scope_dir: Path | None = None) -> str:
239+
"""Run ``git diff <base>..HEAD`` and return the output, sorted by path.
240+
241+
Uses the same base-ref detection logic as changed-file detection.
242+
When *scope_dir* is provided and differs from *project_root*, the
243+
diff is restricted to that subdirectory via ``-- <relpath>``.
244+
The output is sorted by file path so that files in the same
245+
directory are grouped together.
246+
Returns an empty string on failure.
247+
"""
248+
base_ref = _detect_base_ref(project_root)
249+
merge_base = _get_merge_base(project_root, base_ref)
250+
args = ["diff", f"{merge_base}..HEAD"]
251+
if scope_dir is not None and scope_dir != project_root:
252+
try:
253+
rel = scope_dir.relative_to(project_root)
254+
args += ["--", str(rel)]
255+
except ValueError:
256+
pass
257+
try:
258+
result = _run_git(project_root, *args)
259+
return _sort_diff_by_path(result.stdout)
260+
except subprocess.CalledProcessError:
261+
return ""
262+
263+
195264
def match_files_to_rules(
196265
changed_files: list[str],
197266
rules: list[ReviewRule],
@@ -213,6 +282,8 @@ def match_files_to_rules(
213282
List of ReviewTask objects.
214283
"""
215284
tasks: list[ReviewTask] = []
285+
# Lazy-compute diff per source_dir, shared across qualifying rules
286+
_cached_diffs: dict[Path, str] = {}
216287

217288
for rule in rules:
218289
matched = match_rule(changed_files, rule, project_root)
@@ -223,6 +294,14 @@ def match_files_to_rules(
223294
all_filenames = changed_files if rule.all_changed_filenames else None
224295
source_location = format_source_location(rule, project_root)
225296

297+
# Lazily fetch diff for broad rules to reduce reviewer turn count
298+
diff_output: str | None = None
299+
if _should_inject_diff(rule):
300+
scope = rule.source_dir
301+
if scope not in _cached_diffs:
302+
_cached_diffs[scope] = _get_git_diff(project_root, scope)
303+
diff_output = _cached_diffs[scope] or None
304+
226305
if rule.strategy == "individual":
227306
for filepath in matched:
228307
tasks.append(
@@ -249,6 +328,7 @@ def match_files_to_rules(
249328
source_location=source_location,
250329
additional_files=additional,
251330
all_changed_filenames=all_filenames,
331+
git_diff_output=diff_output,
252332
)
253333
)
254334

@@ -261,6 +341,7 @@ def match_files_to_rules(
261341
agent_name=agent_name,
262342
source_location=source_location,
263343
all_changed_filenames=all_filenames,
344+
git_diff_output=diff_output,
264345
)
265346
)
266347

tests/unit/review/test_instructions.py

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ def _make_task(
1818
source_location: str = ".deepreview:1",
1919
additional_files: list[str] | None = None,
2020
all_changed_filenames: list[str] | None = None,
21+
git_diff_output: str | None = None,
2122
) -> ReviewTask:
2223
"""Create a ReviewTask with sensible defaults."""
2324
return ReviewTask(
@@ -28,6 +29,7 @@ def _make_task(
2829
source_location=source_location,
2930
additional_files=additional_files or [],
3031
all_changed_filenames=all_changed_filenames,
32+
git_diff_output=git_diff_output,
3133
)
3234

3335

@@ -138,6 +140,35 @@ def test_traceability_is_at_end(self) -> None:
138140
last_nonblank = [line for line in content.strip().split("\n") if line.strip()][-1]
139141
assert "This review was requested" in last_nonblank
140142

143+
# THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.7.1, REVIEW-REQ-005.7.2).
144+
# YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES
145+
def test_includes_git_diff_section(self) -> None:
146+
task = _make_task(git_diff_output="diff --git a/f.py b/f.py\n+hello\n")
147+
content = build_instruction_file(task)
148+
assert "## Output from `git diff main..HEAD` for you to review" in content
149+
assert "```diff" in content
150+
assert "+hello" in content
151+
152+
# THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.7.4).
153+
# YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES
154+
def test_omits_git_diff_section_when_none(self) -> None:
155+
task = _make_task(git_diff_output=None)
156+
content = build_instruction_file(task)
157+
assert "git diff" not in content
158+
159+
# THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-005.7.3).
160+
# YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES
161+
def test_git_diff_section_after_files_before_all_changed(self) -> None:
162+
task = _make_task(
163+
git_diff_output="diff output",
164+
all_changed_filenames=["a.py", "b.py"],
165+
)
166+
content = build_instruction_file(task)
167+
files_idx = content.index("## Files to Review")
168+
diff_idx = content.index("## Output from `git diff main..HEAD`")
169+
changed_idx = content.index("## All Changed Files")
170+
assert files_idx < diff_idx < changed_idx
171+
141172
# THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-009.4.1, REVIEW-REQ-009.4.3).
142173
# YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES
143174
def test_includes_after_review_section(self) -> None:

0 commit comments

Comments
 (0)