Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion doc/architecture.md
Original file line number Diff line number Diff line change
Expand Up @@ -937,7 +937,7 @@ Runs the `.deepreview`-based code review pipeline. Registered directly in `jobs/
The `--platform` CLI option on `serve` controls which formatter is used (defaults to `"claude"`).

#### 7. `get_configured_reviews`
Lists configured review rules from `.deepreview` files without running the pipeline.
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.

**Parameters**:
- `only_rules_matching_files: list[str] | None` - Filter to rules matching these files.
Expand Down
1 change: 1 addition & 0 deletions specs/deepwork/review/REVIEW-REQ-006-cli-review-command.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,4 @@ The command supports three sources for the list of files to review, with the fol
In all cases:
4. The resulting file list MUST be sorted and deduplicated.
5. The `--base-ref` option only applies to the git diff source; it MUST be ignored when `--files` or stdin provides the file list.
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.
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ The `get_configured_reviews` MCP tool allows agents to query which review rules

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.
2. When `only_rules_matching_files` is omitted (None), the tool MUST return all configured rules.
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.
4. When `only_rules_matching_files` is omitted (None), catch-all rules MUST be retained in the result.

### REVIEW-REQ-008.4: Error Handling

Expand Down
29 changes: 28 additions & 1 deletion src/deepwork/review/mcp.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,22 @@
SUPPORTED_PLATFORMS = set(FORMATTERS.keys())


def _is_catch_all_pattern(pattern: str) -> bool:
"""Return True if a glob pattern has no literal characters (only ``*``/``/``).

Examples: ``*``, ``**``, ``**/*``, ``*/**``, ``**/**`` — patterns that
match effectively every file under the rule's source directory.
"""
return pattern != "" and all(c in "*/" for c in pattern)


def _rule_is_catch_all(rule) -> bool: # type: ignore[no-untyped-def]
"""Return True if every include pattern on the rule is a catch-all."""
return bool(rule.include_patterns) and all(
_is_catch_all_pattern(p) for p in rule.include_patterns
)


class ReviewToolError(Exception):
"""Exception raised for review tool errors (git failures, write failures)."""

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

if only_rules_matching_files is not None:
# Exclude catch-all rules (e.g. ``**/*``) — when the caller has
# narrowed scope to specific files, rules that match everything are
# rarely what they want.
rules = [
rule for rule in rules if match_rule(only_rules_matching_files, rule, project_root)
rule
for rule in rules
if not _rule_is_catch_all(rule)
and match_rule(only_rules_matching_files, rule, project_root)
]

result = [
Expand Down
105 changes: 105 additions & 0 deletions tests/unit/review/test_mcp.py
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,111 @@ def test_discovery_errors_still_return_valid_rules(
assert len(error_entries) == 1


def _make_catch_all_rule(
tmp_path: Path, name: str = "catch_all", pattern: str = "**/*"
) -> ReviewRule:
return ReviewRule(
name=name,
description="Catch-all rule.",
include_patterns=[pattern],
exclude_patterns=[],
strategy="individual",
instructions="Review all.",
agent=None,
all_changed_filenames=False,
unchanged_matching_files=False,
precomputed_info_bash_command=None,
source_dir=tmp_path,
source_file=tmp_path / ".deepreview",
source_line=1,
)


class TestCatchAllFiltering:
"""When files are specified, rules with pure-wildcard include patterns are dropped."""

@patch("deepwork.review.mcp.gen_schema_rules", return_value=([], []))
@patch("deepwork.review.mcp.load_all_rules")
def test_get_configured_reviews_drops_catch_all_when_files_specified(
self, mock_load: Any, mock_schema: Any, tmp_path: Path
) -> None:
# THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-008.3.3).
# YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES
specific_rule = _make_rule(tmp_path) # **/*.py
mock_load.return_value = (
[
specific_rule,
_make_catch_all_rule(tmp_path, "all_star", "*"),
_make_catch_all_rule(tmp_path, "all_double", "**"),
_make_catch_all_rule(tmp_path, "all_nested", "**/*"),
_make_catch_all_rule(tmp_path, "all_trailing", "*/**"),
],
[],
)

result = get_configured_reviews(tmp_path, only_rules_matching_files=["src/app.py"])
names = [r["name"] for r in result]
assert names == ["test_rule"]

@patch("deepwork.review.mcp.gen_schema_rules", return_value=([], []))
@patch("deepwork.review.mcp.load_all_rules")
def test_get_configured_reviews_keeps_catch_all_when_no_files_specified(
self, mock_load: Any, mock_schema: Any, tmp_path: Path
) -> None:
# THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-008.3.4).
# YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES
mock_load.return_value = ([_make_catch_all_rule(tmp_path)], [])
result = get_configured_reviews(tmp_path, only_rules_matching_files=None)
assert len(result) == 1
assert result[0]["name"] == "catch_all"

@patch("deepwork.review.mcp.write_instruction_files", return_value={})
@patch("deepwork.review.mcp.format_for_claude", return_value="ok")
@patch("deepwork.review.mcp.gen_schema_rules", return_value=([], []))
@patch("deepwork.review.mcp.load_all_rules")
def test_run_review_drops_catch_all_when_files_specified(
self,
mock_load: Any,
mock_schema: Any,
mock_fmt: Any,
mock_write: Any,
tmp_path: Path,
) -> None:
# THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-006.6.6).
# YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES
specific_rule = _make_rule(tmp_path)
catch_all = _make_catch_all_rule(tmp_path)
mock_load.return_value = ([specific_rule, catch_all], [])

with patch("deepwork.review.mcp.match_files_to_rules") as mock_match:
mock_match.return_value = []
run_review(tmp_path, "claude", files=["src/app.py"])
passed_rules = mock_match.call_args[0][1]
names = [r.name for r in passed_rules]
assert names == ["test_rule"]

@patch("deepwork.review.mcp.get_changed_files", return_value=["src/app.py"])
@patch("deepwork.review.mcp.gen_schema_rules", return_value=([], []))
@patch("deepwork.review.mcp.load_all_rules")
def test_run_review_keeps_catch_all_when_using_git_diff(
self,
mock_load: Any,
mock_schema: Any,
mock_diff: Any,
tmp_path: Path,
) -> None:
# THIS TEST VALIDATES A HARD REQUIREMENT (REVIEW-REQ-006.6.6).
# YOU MUST NOT MODIFY THIS TEST UNLESS THE REQUIREMENT CHANGES
catch_all = _make_catch_all_rule(tmp_path)
mock_load.return_value = ([catch_all], [])

with patch("deepwork.review.mcp.match_files_to_rules") as mock_match:
mock_match.return_value = []
run_review(tmp_path, "claude", files=None)
passed_rules = mock_match.call_args[0][1]
assert [r.name for r in passed_rules] == ["catch_all"]


class TestMarkPassed:
"""Tests for the mark_passed adapter function — validates REVIEW-REQ-009.2."""

Expand Down
Loading