Merge pull request #71 from codesensei-tushar/fix/file-pattern-changed-files

fix: implement _get_changed_files for FilePatternCondition

Author: dkargatzis

CHANGELOG.md

@@ -8,10 +8,22 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 
 ### Fixed
 
+- **`FilePatternCondition._get_changed_files` implementation** -- Replaced
+  stub that always returned `[]` with a working implementation that extracts
+  file paths from enriched PR data (`changed_files` list of dicts or plain
+  strings) and push event commits (`added`/`modified`/`removed` arrays with
+  deduplication). Added unit tests covering all extraction paths.
+
+## [2026-04-12] -- PR #69
+
+### Fixed
+
 - **Blocking sleep in LLM condition** -- Replaced `time.sleep()` with
   `await asyncio.sleep()` in `LLMAssisted` retry backoff to avoid
   freezing the event loop during LLM retries.
 
+## [2026-04-08] -- PR #66
+
 ### Added
 
 - **AI-powered reviewer recommendation** -- `/reviewers` slash command suggests

src/rules/conditions/filesystem.py

@@ -116,16 +116,33 @@ async def validate(self, parameters: dict[str, Any], event: dict[str, Any]) -> b
             return len(matching_files) > 0
 
     def _get_changed_files(self, event: dict[str, Any]) -> list[str]:
-        """Extract the list of changed files from the event."""
-        event_type = event.get("event_type", "")
-        if event_type == "pull_request":
-            # TODO: Pull request—fetch changed files via GitHub API. Placeholder for now.
-            return []
-        elif event_type == "push":
-            # Push event—files in commits, not implemented.
-            return []
-        else:
-            return []
+        """Extract changed file paths from enriched PR data or push commits."""
+        changed_files = event.get("changed_files", [])
+        if isinstance(changed_files, list) and changed_files:
+            extracted: list[str] = []
+            for item in changed_files:
+                path = item.get("filename") if isinstance(item, dict) else item
+                if isinstance(path, str) and path:
+                    extracted.append(path)
+            if extracted:
+                return extracted
+
+        commits = event.get("commits", [])
+        if isinstance(commits, list) and commits:
+            seen: set[str] = set()
+            for commit in commits:
+                if not isinstance(commit, dict):
+                    continue
+                for key in ("added", "modified", "removed"):
+                    paths = commit.get(key, [])
+                    if not isinstance(paths, list):
+                        continue
+                    for path in paths:
+                        if isinstance(path, str) and path:
+                            seen.add(path)
+            return sorted(seen)
+
+        return []
 
     @staticmethod
     def _glob_to_regex(glob_pattern: str) -> str:

tests/unit/rules/conditions/test_filesystem.py

@@ -3,6 +3,7 @@
 Tests for FilePatternCondition, MaxFileSizeCondition, and MaxPrLocCondition classes.
 """
 
+from typing import Any
 from unittest.mock import patch
 
 import pytest
@@ -96,6 +97,104 @@ def test_glob_to_regex_conversion(self) -> None:
         assert FilePatternCondition._glob_to_regex("src/*.js") == "^src/.*\\.js$"
         assert FilePatternCondition._glob_to_regex("file?.txt") == "^file.\\.txt$"
 
+    def test_get_changed_files_from_pr_enriched_data(self) -> None:
+        """Test extracting files from enriched PR changed_files (list of dicts)."""
+        condition = FilePatternCondition()
+        event = {
+            "changed_files": [
+                {"filename": "src/main.py", "status": "modified", "additions": 10, "deletions": 2},
+                {"filename": "tests/test_main.py", "status": "added", "additions": 30, "deletions": 0},
+            ]
+        }
+        result = condition._get_changed_files(event)
+        assert result == ["src/main.py", "tests/test_main.py"]
+
+    def test_get_changed_files_from_pr_plain_strings(self) -> None:
+        """Test extracting files when changed_files contains plain strings."""
+        condition = FilePatternCondition()
+        event = {"changed_files": ["src/main.py", "README.md"]}
+        result = condition._get_changed_files(event)
+        assert result == ["src/main.py", "README.md"]
+
+    def test_get_changed_files_from_push_commits(self) -> None:
+        """Test extracting files from push event commit arrays."""
+        condition = FilePatternCondition()
+        event = {
+            "commits": [
+                {"added": ["new_file.py"], "modified": ["src/main.py"], "removed": []},
+                {"added": [], "modified": ["src/main.py"], "removed": ["old.py"]},
+            ]
+        }
+        result = condition._get_changed_files(event)
+        assert result == ["new_file.py", "old.py", "src/main.py"]
+
+    def test_get_changed_files_empty_event(self) -> None:
+        """Test that an empty event returns no files."""
+        condition = FilePatternCondition()
+        assert condition._get_changed_files({}) == []
+
+    def test_get_changed_files_with_malformed_payload(self) -> None:
+        """Test that malformed payload entries are filtered out without raising."""
+        condition = FilePatternCondition()
+
+        # changed_files with mixed valid/invalid entries
+        event_cf: dict[str, Any] = {
+            "changed_files": [
+                {"filename": "valid.py", "status": "modified"},
+                {"status": "added"},  # missing "filename"
+                None,  # type: ignore[list-item]
+                42,  # type: ignore[list-item]
+                "",  # empty string
+                {"filename": ""},  # empty filename
+                "also_valid.txt",
+            ]
+        }
+        result = condition._get_changed_files(event_cf)
+        assert result == ["valid.py", "also_valid.txt"]
+
+        # commits with non-dict entries and non-list/non-string values
+        event_commits: dict[str, Any] = {
+            "commits": [
+                {"added": ["good.py"], "modified": "not_a_list", "removed": [42, None, "removed.py"]},
+                "not_a_dict",  # type: ignore[list-item]
+                {"added": [None, "", "another.py"], "modified": [], "removed": []},
+            ]
+        }
+        result = condition._get_changed_files(event_commits)
+        assert result == ["another.py", "good.py", "removed.py"]
+
+    @pytest.mark.asyncio
+    async def test_evaluate_with_real_pr_event(self) -> None:
+        """Test full evaluate flow with enriched PR data (no mocking)."""
+        condition = FilePatternCondition()
+        context = {
+            "parameters": {"pattern": "*.py", "condition_type": "files_match_pattern"},
+            "event": {
+                "changed_files": [
+                    {"filename": "src/app.py", "status": "modified", "additions": 5, "deletions": 1},
+                    {"filename": "docs/readme.md", "status": "modified", "additions": 2, "deletions": 0},
+                ]
+            },
+        }
+        violations = await condition.evaluate(context)
+        assert len(violations) == 0
+
+    @pytest.mark.asyncio
+    async def test_evaluate_with_real_push_event(self) -> None:
+        """Test full evaluate flow with push commit data (no mocking)."""
+        condition = FilePatternCondition()
+        context = {
+            "parameters": {"pattern": "*.yaml", "condition_type": "files_not_match_pattern"},
+            "event": {
+                "commits": [
+                    {"added": ["config/app.yaml"], "modified": [], "removed": []},
+                ]
+            },
+        }
+        violations = await condition.evaluate(context)
+        assert len(violations) == 1
+        assert "forbidden pattern" in violations[0].message
+
 
 class TestMaxFileSizeCondition:
     """Tests for MaxFileSizeCondition class."""