From aeff691efba81fa0d0fdd012c4425860f569feb5 Mon Sep 17 00:00:00 2001 From: codesensei-tushar Date: Thu, 9 Apr 2026 09:57:17 +0530 Subject: [PATCH 1/4] fix: implement _get_changed_files for FilePatternCondition --- src/rules/conditions/filesystem.py | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/src/rules/conditions/filesystem.py b/src/rules/conditions/filesystem.py index 2a124ba..dc3a4ac 100644 --- a/src/rules/conditions/filesystem.py +++ b/src/rules/conditions/filesystem.py @@ -116,16 +116,25 @@ 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 changed_files: + return [ + f["filename"] if isinstance(f, dict) else f + for f in changed_files + if (f.get("filename") if isinstance(f, dict) else f) + ] + + commits = event.get("commits", []) + if commits: + seen: set[str] = set() + for commit in commits: + for key in ("added", "modified", "removed"): + for path in commit.get(key, []): + seen.add(path) + return sorted(seen) + + return [] @staticmethod def _glob_to_regex(glob_pattern: str) -> str: From be60ed0633d0004005d2a58c5c5f563d994330c7 Mon Sep 17 00:00:00 2001 From: codesensei-tushar Date: Thu, 9 Apr 2026 09:57:26 +0530 Subject: [PATCH 2/4] test: add unit tests for _get_changed_files implementation --- .../unit/rules/conditions/test_filesystem.py | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/tests/unit/rules/conditions/test_filesystem.py b/tests/unit/rules/conditions/test_filesystem.py index c729292..8c40ea6 100644 --- a/tests/unit/rules/conditions/test_filesystem.py +++ b/tests/unit/rules/conditions/test_filesystem.py @@ -96,6 +96,74 @@ 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({}) == [] + + @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.""" From 6ffde344cc0a1d41372bc76c4f139be81711d56c Mon Sep 17 00:00:00 2001 From: codesensei-tushar Date: Thu, 9 Apr 2026 10:02:10 +0530 Subject: [PATCH 3/4] docs: update changelog for _get_changed_files fix --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 275a6b9..018a586 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## [Unreleased] +### 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. + ### Added - **AI-powered reviewer recommendation** -- `/reviewers` slash command suggests From 1ba494fad47fd120c52c30ca1c2d5c83b33c4098 Mon Sep 17 00:00:00 2001 From: codesensei-tushar Date: Thu, 9 Apr 2026 10:14:36 +0530 Subject: [PATCH 4/4] fix: add type guards to _get_changed_files payload parsing --- src/rules/conditions/filesystem.py | 26 ++++++++++------ .../unit/rules/conditions/test_filesystem.py | 31 +++++++++++++++++++ 2 files changed, 48 insertions(+), 9 deletions(-) diff --git a/src/rules/conditions/filesystem.py b/src/rules/conditions/filesystem.py index dc3a4ac..38bf675 100644 --- a/src/rules/conditions/filesystem.py +++ b/src/rules/conditions/filesystem.py @@ -118,20 +118,28 @@ async def validate(self, parameters: dict[str, Any], event: dict[str, Any]) -> b def _get_changed_files(self, event: dict[str, Any]) -> list[str]: """Extract changed file paths from enriched PR data or push commits.""" changed_files = event.get("changed_files", []) - if changed_files: - return [ - f["filename"] if isinstance(f, dict) else f - for f in changed_files - if (f.get("filename") if isinstance(f, dict) else f) - ] + 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 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"): - for path in commit.get(key, []): - seen.add(path) + 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 [] diff --git a/tests/unit/rules/conditions/test_filesystem.py b/tests/unit/rules/conditions/test_filesystem.py index 8c40ea6..1870ac2 100644 --- a/tests/unit/rules/conditions/test_filesystem.py +++ b/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 @@ -132,6 +133,36 @@ def test_get_changed_files_empty_event(self) -> None: 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)."""