Skip to content

Commit 8ac1258

Browse files
nhortonclaude
andcommitted
feat(review): exclude catch-all rules when files are specified
When callers narrow review scope to a specific file set (via run_review files= or get_configured_reviews only_rules_matching_files=), rules whose include patterns are all pure wildcards (*, **, **/*, */**) are now dropped — they rarely reflect caller intent and produce noisy results. Rules with any literal path segment are kept. Catch-all rules remain in effect when scope comes from git diff. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent b872dc9 commit 8ac1258

5 files changed

Lines changed: 137 additions & 2 deletions

File tree

doc/architecture.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -937,7 +937,7 @@ Runs the `.deepreview`-based code review pipeline. Registered directly in `jobs/
937937
The `--platform` CLI option on `serve` controls which formatter is used (defaults to `"claude"`).
938938
939939
#### 7. `get_configured_reviews`
940-
Lists configured review rules from `.deepreview` files without running the pipeline.
940+
Lists configured review rules from `.deepreview` files and DeepSchema-generated synthetic rules without running the pipeline. When `only_rules_matching_files` is provided, catch-all rules (include patterns composed solely of `*`/`/`, e.g. `**/*`) are excluded.
941941
942942
**Parameters**:
943943
- `only_rules_matching_files: list[str] | None` - Filter to rules matching these files.

specs/deepwork/review/REVIEW-REQ-006-cli-review-command.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,3 +60,4 @@ The command supports three sources for the list of files to review, with the fol
6060
In all cases:
6161
4. The resulting file list MUST be sorted and deduplicated.
6262
5. The `--base-ref` option only applies to the git diff source; it MUST be ignored when `--files` or stdin provides the file list.
63+
6. When the file list is supplied explicitly (via `--files` or stdin), rules whose include patterns are all catch-alls (composed solely of `*` and `/` characters, e.g. `*`, `**`, `**/*`, `*/**`) MUST be excluded from matching. When the file list comes from git diff, catch-all rules MUST be retained. Rationale: when a caller narrows scope to a specific file set, catch-all rules are rarely the intent and produce noisy results.

specs/deepwork/review/REVIEW-REQ-008-get-configured-reviews.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ The `get_configured_reviews` MCP tool allows agents to query which review rules
2323

2424
1. When `only_rules_matching_files` is provided, the tool MUST return only rules whose include/exclude patterns match at least one of the provided files.
2525
2. When `only_rules_matching_files` is omitted (None), the tool MUST return all configured rules.
26+
3. When `only_rules_matching_files` is provided, the tool MUST exclude rules whose include patterns are all catch-alls (patterns composed solely of `*` and `/` characters, e.g. `*`, `**`, `**/*`, `*/**`). Rationale: when the caller has narrowed scope to specific files, rules that match every file in the repo are rarely what is wanted and produce noisy results.
27+
4. When `only_rules_matching_files` is omitted (None), catch-all rules MUST be retained in the result.
2628

2729
### REVIEW-REQ-008.4: Error Handling
2830

src/deepwork/review/mcp.py

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,22 @@
2525
SUPPORTED_PLATFORMS = set(FORMATTERS.keys())
2626

2727

28+
def _is_catch_all_pattern(pattern: str) -> bool:
29+
"""Return True if a glob pattern has no literal characters (only ``*``/``/``).
30+
31+
Examples: ``*``, ``**``, ``**/*``, ``*/**``, ``**/**`` — patterns that
32+
match effectively every file under the rule's source directory.
33+
"""
34+
return pattern != "" and all(c in "*/" for c in pattern)
35+
36+
37+
def _rule_is_catch_all(rule) -> bool: # type: ignore[no-untyped-def]
38+
"""Return True if every include pattern on the rule is a catch-all."""
39+
return bool(rule.include_patterns) and all(
40+
_is_catch_all_pattern(p) for p in rule.include_patterns
41+
)
42+
43+
2844
class ReviewToolError(Exception):
2945
"""Exception raised for review tool errors (git failures, write failures)."""
3046

@@ -85,6 +101,11 @@ def run_review(
85101
# Step 2: Determine changed files
86102
if files is not None:
87103
changed_files = sorted(set(files))
104+
# When the caller specifies files explicitly, drop rules whose
105+
# include patterns are pure catch-alls (e.g. ``**/*``). Those rules
106+
# would match every file and are rarely what the caller wants when
107+
# they have narrowed the scope to a specific file set.
108+
rules = [r for r in rules if not _rule_is_catch_all(r)]
88109
else:
89110
try:
90111
changed_files = get_changed_files(project_root)
@@ -138,8 +159,14 @@ def get_configured_reviews(
138159
rules.extend(schema_rules)
139160

140161
if only_rules_matching_files is not None:
162+
# Exclude catch-all rules (e.g. ``**/*``) — when the caller has
163+
# narrowed scope to specific files, rules that match everything are
164+
# rarely what they want.
141165
rules = [
142-
rule for rule in rules if match_rule(only_rules_matching_files, rule, project_root)
166+
rule
167+
for rule in rules
168+
if not _rule_is_catch_all(rule)
169+
and match_rule(only_rules_matching_files, rule, project_root)
143170
]
144171

145172
result = [

tests/unit/review/test_mcp.py

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -469,6 +469,111 @@ def test_discovery_errors_still_return_valid_rules(
469469
assert len(error_entries) == 1
470470

471471

472+
def _make_catch_all_rule(
473+
tmp_path: Path, name: str = "catch_all", pattern: str = "**/*"
474+
) -> ReviewRule:
475+
return ReviewRule(
476+
name=name,
477+
description="Catch-all rule.",
478+
include_patterns=[pattern],
479+
exclude_patterns=[],
480+
strategy="individual",
481+
instructions="Review all.",
482+
agent=None,
483+
all_changed_filenames=False,
484+
unchanged_matching_files=False,
485+
precomputed_info_bash_command=None,
486+
source_dir=tmp_path,
487+
source_file=tmp_path / ".deepreview",
488+
source_line=1,
489+
)
490+
491+
492+
class TestCatchAllFiltering:
493+
"""When files are specified, rules with pure-wildcard include patterns are dropped."""
494+
495+
@patch("deepwork.review.mcp.gen_schema_rules", return_value=([], []))
496+
@patch("deepwork.review.mcp.load_all_rules")
497+
def test_get_configured_reviews_drops_catch_all_when_files_specified(
498+
self, mock_load: Any, mock_schema: Any, tmp_path: Path
499+
) -> None:
500+
# THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-008.3.3).
501+
# YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES
502+
specific_rule = _make_rule(tmp_path) # **/*.py
503+
mock_load.return_value = (
504+
[
505+
specific_rule,
506+
_make_catch_all_rule(tmp_path, "all_star", "*"),
507+
_make_catch_all_rule(tmp_path, "all_double", "**"),
508+
_make_catch_all_rule(tmp_path, "all_nested", "**/*"),
509+
_make_catch_all_rule(tmp_path, "all_trailing", "*/**"),
510+
],
511+
[],
512+
)
513+
514+
result = get_configured_reviews(tmp_path, only_rules_matching_files=["src/app.py"])
515+
names = [r["name"] for r in result]
516+
assert names == ["test_rule"]
517+
518+
@patch("deepwork.review.mcp.gen_schema_rules", return_value=([], []))
519+
@patch("deepwork.review.mcp.load_all_rules")
520+
def test_get_configured_reviews_keeps_catch_all_when_no_files_specified(
521+
self, mock_load: Any, mock_schema: Any, tmp_path: Path
522+
) -> None:
523+
# THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-008.3.4).
524+
# YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES
525+
mock_load.return_value = ([_make_catch_all_rule(tmp_path)], [])
526+
result = get_configured_reviews(tmp_path, only_rules_matching_files=None)
527+
assert len(result) == 1
528+
assert result[0]["name"] == "catch_all"
529+
530+
@patch("deepwork.review.mcp.write_instruction_files", return_value={})
531+
@patch("deepwork.review.mcp.format_for_claude", return_value="ok")
532+
@patch("deepwork.review.mcp.gen_schema_rules", return_value=([], []))
533+
@patch("deepwork.review.mcp.load_all_rules")
534+
def test_run_review_drops_catch_all_when_files_specified(
535+
self,
536+
mock_load: Any,
537+
mock_schema: Any,
538+
mock_fmt: Any,
539+
mock_write: Any,
540+
tmp_path: Path,
541+
) -> None:
542+
# THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-006.6.6).
543+
# YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES
544+
specific_rule = _make_rule(tmp_path)
545+
catch_all = _make_catch_all_rule(tmp_path)
546+
mock_load.return_value = ([specific_rule, catch_all], [])
547+
548+
with patch("deepwork.review.mcp.match_files_to_rules") as mock_match:
549+
mock_match.return_value = []
550+
run_review(tmp_path, "claude", files=["src/app.py"])
551+
passed_rules = mock_match.call_args[0][1]
552+
names = [r.name for r in passed_rules]
553+
assert names == ["test_rule"]
554+
555+
@patch("deepwork.review.mcp.get_changed_files", return_value=["src/app.py"])
556+
@patch("deepwork.review.mcp.gen_schema_rules", return_value=([], []))
557+
@patch("deepwork.review.mcp.load_all_rules")
558+
def test_run_review_keeps_catch_all_when_using_git_diff(
559+
self,
560+
mock_load: Any,
561+
mock_schema: Any,
562+
mock_diff: Any,
563+
tmp_path: Path,
564+
) -> None:
565+
# THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-006.6.6).
566+
# YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES
567+
catch_all = _make_catch_all_rule(tmp_path)
568+
mock_load.return_value = ([catch_all], [])
569+
570+
with patch("deepwork.review.mcp.match_files_to_rules") as mock_match:
571+
mock_match.return_value = []
572+
run_review(tmp_path, "claude", files=None)
573+
passed_rules = mock_match.call_args[0][1]
574+
assert [r.name for r in passed_rules] == ["catch_all"]
575+
576+
472577
class TestMarkPassed:
473578
"""Tests for the mark_passed adapter function — validates REVIEW-REQ-009.2."""
474579

0 commit comments

Comments
 (0)