Skip to content

Commit b995a1c

Browse files
nhortonclaude
andcommitted
revert: remove git_diff injection for broad review rules
The git_diff_output feature on ReviewTask (REVIEW-REQ-004.11, REVIEW-REQ-005.7) added complexity without sufficient benefit. Removes the field, helper functions, spec requirements, and tests. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 28e50bc commit b995a1c

7 files changed

Lines changed: 1 addition & 427 deletions

File tree

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

Lines changed: 1 addition & 10 deletions
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), `git_diff_output` (str | None — pre-fetched diff for broad rules, see REVIEW-REQ-004.11).
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).
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,12 +71,3 @@ 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: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,6 @@ 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-
2619
### REVIEW-REQ-005.2: File Path Formatting
2720

2821
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: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,6 @@ 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
5352

5453

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

src/deepwork/review/instructions.py

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -155,14 +155,6 @@ 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-
166158
# Additional context: all changed filenames
167159
if task.all_changed_filenames:
168160
parts.append("## All Changed Files\n")

src/deepwork/review/matcher.py

Lines changed: 0 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -192,75 +192,6 @@ 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-
264195
def match_files_to_rules(
265196
changed_files: list[str],
266197
rules: list[ReviewRule],
@@ -282,8 +213,6 @@ def match_files_to_rules(
282213
List of ReviewTask objects.
283214
"""
284215
tasks: list[ReviewTask] = []
285-
# Lazy-compute diff per source_dir, shared across qualifying rules
286-
_cached_diffs: dict[Path, str] = {}
287216

288217
for rule in rules:
289218
matched = match_rule(changed_files, rule, project_root)
@@ -294,14 +223,6 @@ def match_files_to_rules(
294223
all_filenames = changed_files if rule.all_changed_filenames else None
295224
source_location = format_source_location(rule, project_root)
296225

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-
305226
if rule.strategy == "individual":
306227
for filepath in matched:
307228
tasks.append(
@@ -328,7 +249,6 @@ def match_files_to_rules(
328249
source_location=source_location,
329250
additional_files=additional,
330251
all_changed_filenames=all_filenames,
331-
git_diff_output=diff_output,
332252
)
333253
)
334254

@@ -341,7 +261,6 @@ def match_files_to_rules(
341261
agent_name=agent_name,
342262
source_location=source_location,
343263
all_changed_filenames=all_filenames,
344-
git_diff_output=diff_output,
345264
)
346265
)
347266

tests/unit/review/test_instructions.py

Lines changed: 0 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ 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,
2221
) -> ReviewTask:
2322
"""Create a ReviewTask with sensible defaults."""
2423
return ReviewTask(
@@ -29,7 +28,6 @@ def _make_task(
2928
source_location=source_location,
3029
additional_files=additional_files or [],
3130
all_changed_filenames=all_changed_filenames,
32-
git_diff_output=git_diff_output,
3331
)
3432

3533

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

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-
172141
# THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-009.4.1, REVIEW-REQ-009.4.3).
173142
# YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES
174143
def test_includes_after_review_section(self) -> None:

0 commit comments

Comments
 (0)