Recognize canonical repository renames in README maintenance (#663)

scripts/review_pr.py

@@ -14,6 +14,7 @@ import ssl
 import sys
 from dataclasses import dataclass
 from datetime import datetime, timedelta, timezone
+from functools import cache
 from pathlib import Path
 from typing import Any, Callable
 from urllib.parse import urlsplit, urlunsplit
@@ -180,7 +181,20 @@ def read_readme(repository: Any, ref: str) -> str:
     return content.decoded_content.decode("utf-8")
 
 
-def entries_represent_same_project(old_line: str, new_line: str) -> bool:
+def entry_github_urls(line: str) -> set[str]:
+    match = ENTRY_RE.match(line)
+    if match is None:
+        return set()
+    urls = set(GITHUB_LINK_RE.findall(line))
+    if parse_github_repository_url(match.group(2).strip()):
+        urls.add(match.group(2).strip())
+    return urls
+
+
+def entries_represent_same_project(
+    old_line: str, new_line: str,
+    repository_id: Callable[[str], int | None] | None = None,
+) -> bool:
     old_match = ENTRY_RE.match(old_line)
     new_match = ENTRY_RE.match(new_line)
     if old_match is None or new_match is None:
@@ -195,11 +209,18 @@ def entries_represent_same_project(old_line: str, new_line: str) -> bool:
     new_urls = {
         canonicalize_url(url) for url in MARKDOWN_URL_RE.findall(new_line)
     }
-    return bool(old_urls & new_urls)
+    if old_urls & new_urls:
+        return True
+    if repository_id is None:
+        return False
+    old_ids = {repository_id(url) for url in entry_github_urls(old_line)} - {None}
+    new_ids = {repository_id(url) for url in entry_github_urls(new_line)} - {None}
+    return bool(old_ids & new_ids)
 
 
 def analyze_readme_change(
     patch: str | None,
+    repository_id: Callable[[str], int | None] | None = None,
 ) -> tuple[list[str], list[str], list[Finding]]:
     added_lines: list[str] = []
     removed_lines: list[str] = []
@@ -219,7 +240,9 @@ def analyze_readme_change(
         line for line in substantive_removed if line.strip().startswith("- ")
     ]
     findings: list[Finding] = []
-    previous_entries = match_existing_entries(entry_lines, removed_entry_lines)
+    previous_entries = match_existing_entries(
+        entry_lines, removed_entry_lines, repository_id
+    )
     maintenance = bool(removed_entry_lines) and all(
         previous is not None for previous in previous_entries
     )
@@ -259,13 +282,17 @@ def analyze_readme_change(
 
 def match_existing_entries(
     added: list[str], removed: list[str],
+    repository_id: Callable[[str], int | None] | None = None,
 ) -> list[str | None]:
     """Pair updates one-to-one; an extra copy is still a new entry."""
     unmatched = list(removed)
     previous_entries: list[str | None] = []
     for line in added:
         index = next(
-            (i for i, old in enumerate(unmatched) if entries_represent_same_project(old, line)),
+            (
+                i for i, old in enumerate(unmatched)
+                if entries_represent_same_project(old, line, repository_id)
+            ),
             None,
         )
         previous_entries.append(unmatched.pop(index) if index is not None else None)
@@ -534,6 +561,7 @@ def review_entry(
     readme_cache: dict[str, str],
     previous_line: str | None = None,
     previous_section: str = "",
+    repository_id: Callable[[str], int | None] | None = None,
 ) -> list[Finding]:
     findings: list[Finding] = []
     match = ENTRY_RE.match(line)
@@ -629,6 +657,13 @@ def review_entry(
         canonicalize_url(item) for item in github_urls
     } == {canonicalize_url(item) for item in previous_github_urls}
 
+    if same_section_update and not unchanged_source and repository_id is not None:
+        previous_ids = {repository_id(item) for item in previous_github_urls}
+        current_ids = {repository_id(item) for item in github_urls}
+        unchanged_source = bool(current_ids) and None not in current_ids and (
+            current_ids == previous_ids
+        )
+
     if not github_urls:
         if section != "Commercial & Proprietary Services" and not unchanged_source:
             findings.append(
@@ -731,10 +766,22 @@ def review_pr(
         )
         return findings, pull_request.title, 0
 
+    @cache
+    def repository_id(url: str) -> int | None:
+        parts = parse_github_repository_url(url)
+        if parts is None:
+            return None
+        try:
+            identity = getattr(client.get_repo("/".join(parts)), "id", None)
+        except GithubException:
+            # Unverified identities cannot grant a cleanup exemption.
+            return None
+        return identity if type(identity) is int and identity > 0 else None
+
     base_readme = read_readme(repository, pull_request.base.sha)
     head_readme = read_readme(repository, pull_request.head.sha)
     entry_lines, removed_entry_lines, change_findings = analyze_readme_change(
-        files[0].patch,
+        files[0].patch, repository_id,
     )
     findings.extend(change_findings)
     if not entry_lines:
@@ -745,7 +792,9 @@ def review_pr(
         pull_request.base.sha: base_readme,
         pull_request.head.sha: head_readme,
     }
-    previous_entries = match_existing_entries(entry_lines, removed_entry_lines)
+    previous_entries = match_existing_entries(
+        entry_lines, removed_entry_lines, repository_id
+    )
     maintenance = bool(removed_entry_lines) and all(
         previous is not None for previous in previous_entries
     )
@@ -764,6 +813,7 @@ def review_pr(
                 check_pull_request_duplicates=check_pull_request_duplicates,
                 readme_cache=readme_cache,
                 previous_line=previous_line,
+                repository_id=repository_id,
                 previous_section=(
                     find_entry_section(base_readme, previous_line, require_unique=False)
                     if previous_line

tests/test_review_pr.py

@@ -13,6 +13,7 @@ from scripts.review_pr import (
     Finding,
     PinnedHTTPSConnection,
     main,
+    match_existing_entries,
     readme_has_duplicate,
     review_pr,
     url_reachable,
@@ -620,6 +621,90 @@ class ValidationPipelineTests(unittest.TestCase):
             old, old.replace("Old", "New"), configure_project=missing_docs
         ))
 
+    def test_cleanup_same_id_rename_preserves_legacy_exemption(self):
+        old = "- [Fresh](https://github.com/example/old) - `Python` - Research tool.\n"
+        def legacy(project):
+            project.id = 123
+            project.archived = True
+            project.pushed_at = NOW - timedelta(days=800)
+        self.assertEqual(self.review_cleanup(
+            old, old.replace("example/old", "example/fresh"), configure_project=legacy
+        ), set())
+
+    def test_cleanup_six_name_and_url_renames_match_same_ids(self):
+        old = "".join(
+            f"- [Old {i}](https://github.com/example/old-{i}) - `Python` - Tool.\n"
+            for i in range(6)
+        )
+        new = old.replace("[Old", "[New").replace("/old-", "/new-")
+        original = FakeClient.get_repo
+        requested = []
+        def lookup(client, name):
+            requested.append(name)
+            project = original(client, name)
+            if name.startswith("example/"):
+                project.id = int(name.rsplit("-", 1)[1]) + 1
+            return project
+        with patch.object(FakeClient, "get_repo", lookup):
+            self.assertEqual(self.review_cleanup(old, new), set())
+        for i in range(6):
+            self.assertEqual(requested.count(f"example/old-{i}"), 1)
+
+    def test_cleanup_rename_does_not_pair_extra_copy(self):
+        old = "- [Old](https://github.com/example/old) - `Python` - Tool.\n"
+        new = old.replace("[Old]", "[New]").replace("/old)", "/new)")
+        def identity(project):
+            project.id = 123
+        self.assertEqual(
+            match_existing_entries([new, new], [old], lambda url: 123),
+            [old, None],
+        )
+        self.assertIn("duplicates", self.review_cleanup(
+            old, new + new.replace("Tool.", "Second copy."), configure_project=identity
+        ))
+
+    def test_cleanup_different_ids_still_check_activity(self):
+        old = "- [Fresh](https://github.com/example/old) - `Python` - Tool.\n"
+        original = FakeClient.get_repo
+        def lookup(client, name):
+            project = original(client, name)
+            if name.startswith("example/"):
+                project.id = 1 if name.endswith("/old") else 2
+                project.pushed_at = NOW - timedelta(days=800)
+            return project
+        with patch.object(FakeClient, "get_repo", lookup):
+            self.assertIn("activity", self.review_cleanup(
+                old, old.replace("/old)", "/new)")
+            ))
+
+    def test_cleanup_same_id_rename_section_move_checks_activity(self):
+        old = "- [Old](https://github.com/example/old) - `Python` - Tool.\n"
+        new = old.replace("[Old]", "[New]").replace("/old)", "/new)")
+        def archived(project):
+            project.id = 123
+            project.archived = True
+        self.assertIn("activity", self.review_cleanup(
+            old + "\n## Portfolio Optimization & Risk Analysis\n",
+            "\n## Portfolio Optimization & Risk Analysis\n" + new,
+            configure_project=archived,
+        ))
+
+    def test_cleanup_failed_old_identity_lookup_does_not_grant_exemption(self):
+        old = "- [Fresh](https://github.com/example/old) - `Python` - Tool.\n"
+        original = FakeClient.get_repo
+        def lookup(client, name):
+            if name == "example/old":
+                raise GithubException(404, {"message": "Not Found"})
+            project = original(client, name)
+            if name.startswith("example/"):
+                project.id = 123
+                project.pushed_at = NOW - timedelta(days=800)
+            return project
+        with patch.object(FakeClient, "get_repo", lookup):
+            self.assertIn("activity", self.review_cleanup(
+                old, old.replace("/old)", "/new)")
+            ))
+
     def test_cleanup_rechecks_replacement_repository_activity(self):
         old = "- [Fresh](https://github.com/example/old) - `Python` - Old description.\n"
         def stale(project):