Enhanced assign_reviewers.py with detailed feedback for unmapped files and modules

- Modified map_modules() to return both modules and unmapped files
- Enhanced gather_reviewers() to track modules without owners
- Added comprehensive feedback when no reviewers are assigned:
  - Warns about files with no module mapping
  - Warns about modules with no owners
  - Explains specific reasons for no assignment
  - Provides actionable guidance for fixing coverage gaps
- Updated test cases to cover new functionality
- Added test cases for unmapped files and modules without owners

Signed-off-by: Venky Ganesh <[email protected]>

Author: venkywonka

.github/scripts/assign_reviewers.py

@@ -66,25 +66,43 @@ def load_json(path: str):
         return json.load(f)
 
 
-def map_modules(changed_files: list[str], module_paths: dict[str,
-                                                             str]) -> set[str]:
+def map_modules(changed_files: list[str],
+                module_paths: dict[str, str]) -> tuple[set[str], list[str]]:
+    """Map changed files to modules and return both modules and unmapped files"""
     modules: set[str] = set()
+    unmapped_files: list[str] = []
+
     for file in changed_files:
+        mapped = False
         for prefix, module in module_paths.items():
             if file.startswith(prefix):
                 modules.add(module)
+                mapped = True
                 break
-    return modules
+
+        if not mapped:
+            unmapped_files.append(file)
+
+    return modules, unmapped_files
 
 
-def gather_reviewers(modules: set[str],
-                     module_owners: dict[str, list[str]],
-                     *,
-                     pr_author: str | None = None,
-                     existing_reviewers: set[str] | None = None) -> list[str]:
+def gather_reviewers(
+        modules: set[str],
+        module_owners: dict[str, list[str]],
+        *,
+        pr_author: str | None = None,
+        existing_reviewers: set[str] | None = None
+) -> tuple[list[str], set[str]]:
+    """Gather reviewers and return both reviewers and modules without owners"""
     reviewers: set[str] = set()
+    modules_without_owners: set[str] = set()
+
     for module in modules:
-        reviewers.update(module_owners.get(module, []))
+        owners = module_owners.get(module, [])
+        if owners:
+            reviewers.update(owners)
+        else:
+            modules_without_owners.add(module)
 
     if pr_author:
         reviewers.discard(pr_author)
@@ -93,7 +111,7 @@ def gather_reviewers(modules: set[str],
     if existing_reviewers:
         reviewers -= existing_reviewers
 
-    return sorted(reviewers)
+    return sorted(reviewers), modules_without_owners
 
 
 def main() -> None:
@@ -139,8 +157,8 @@ def main() -> None:
         module_paths = load_json(Path(".github") / "module-paths.json")
         module_owners = load_json(Path(".github") / "module-owners.json")
 
-        modules = map_modules(changed_files, module_paths)
-        reviewers = gather_reviewers(
+        modules, unmapped_files = map_modules(changed_files, module_paths)
+        reviewers, modules_without_owners = gather_reviewers(
             modules,
             module_owners,
             pr_author=pr_author,
@@ -154,6 +172,23 @@ def main() -> None:
         print(f"Changed modules: {sorted(modules)}")
         print(f"Potential reviewers: {reviewers}")
 
+        # Provide detailed feedback about coverage gaps
+        if unmapped_files:
+            print(f"⚠️  Files with no module mapping: {unmapped_files}")
+            print(
+                f"   These files are not covered in .github/module-paths.json")
+            print(
+                f"   Consider adding appropriate module mappings for these paths."
+            )
+
+        if modules_without_owners:
+            print(
+                f"⚠️  Modules with no owners: {sorted(modules_without_owners)}")
+            print(
+                f"   These modules exist in module-paths.json but have no owners in module-owners.json"
+            )
+            print(f"   Consider adding owner assignments for these modules.")
+
         if reviewers:
             cmd = ["gh", "pr", "edit", pr_number]
             for reviewer in reviewers:
@@ -176,6 +211,31 @@ def main() -> None:
         else:
             print("✅ No new reviewers to assign")
 
+            # Explain why no reviewers were assigned
+            if not modules and not unmapped_files:
+                print("   Reason: No files were changed in this PR")
+            elif not modules and unmapped_files:
+                print(
+                    "   Reason: All changed files are unmapped (no module coverage)"
+                )
+                print(
+                    "   ➜ Action needed: Add module mappings to .github/module-paths.json"
+                )
+            elif modules and not reviewers:
+                if modules_without_owners:
+                    print("   Reason: Matched modules have no assigned owners")
+                    print(
+                        "   ➜ Action needed: Add owner assignments to .github/module-owners.json"
+                    )
+                else:
+                    print(
+                        "   Reason: All potential reviewers are already assigned or excluded"
+                    )
+            else:
+                print(
+                    "   Reason: Complex combination of mapping/ownership issues (see warnings above)"
+                )
+
     except subprocess.CalledProcessError as e:
         print(f"❌ Error processing PR: {e}", file=sys.stderr)
         sys.exit(1)

.github/scripts/tests/test_assign_reviewers.py

@@ -337,29 +337,54 @@ def test_map_modules_function(self):
             "cpp/main.cpp", "cpp/utils.h", "docs/README.md", "unknown/file.txt"
         ]
 
-        modules = assign_reviewers.map_modules(changed_files, self.module_paths)
+        modules, unmapped_files = assign_reviewers.map_modules(
+            changed_files, self.module_paths)
 
         self.assertEqual(modules, {"Generic Runtime", "Documentation"})
+        self.assertEqual(unmapped_files, ["unknown/file.txt"])
 
     def test_gather_reviewers_function(self):
         """Test the pure gather_reviewers function"""
         modules = {"Generic Runtime", "Documentation"}
 
         # Test without exclusions
-        reviewers = assign_reviewers.gather_reviewers(modules,
-                                                      self.module_owners)
+        reviewers, modules_without_owners = assign_reviewers.gather_reviewers(
+            modules, self.module_owners)
         self.assertEqual(set(reviewers), {"user1", "user2", "user3", "user9"})
+        self.assertEqual(modules_without_owners, set())
 
         # Test with author exclusion
-        reviewers = assign_reviewers.gather_reviewers(modules,
-                                                      self.module_owners,
-                                                      pr_author="user1")
+        reviewers, modules_without_owners = assign_reviewers.gather_reviewers(
+            modules, self.module_owners, pr_author="user1")
         self.assertEqual(set(reviewers), {"user2", "user3", "user9"})
+        self.assertEqual(modules_without_owners, set())
 
         # Test with existing reviewers exclusion
-        reviewers = assign_reviewers.gather_reviewers(
+        reviewers, modules_without_owners = assign_reviewers.gather_reviewers(
             modules, self.module_owners, existing_reviewers={"user2", "user9"})
         self.assertEqual(set(reviewers), {"user1", "user3"})
+        self.assertEqual(modules_without_owners, set())
+
+    def test_modules_without_owners(self):
+        """Test modules that have no owners defined"""
+        modules = {"Generic Runtime", "NonExistent Module"}
+
+        reviewers, modules_without_owners = assign_reviewers.gather_reviewers(
+            modules, self.module_owners)
+
+        self.assertEqual(set(reviewers), {"user1", "user2", "user3"})
+        self.assertEqual(modules_without_owners, {"NonExistent Module"})
+
+    def test_all_files_unmapped(self):
+        """Test when all files are unmapped"""
+        changed_files = ["unmapped/file1.txt", "another/file2.py"]
+
+        modules, unmapped_files = assign_reviewers.map_modules(
+            changed_files, self.module_paths)
+
+        self.assertEqual(modules, set())
+        self.assertEqual(set(unmapped_files),
+                         {"unmapped/file1.txt", "another/file2.py"})
 
     @patch('assign_reviewers.load_json')
     @patch('subprocess.run')