Support README audit cleanup in PR review

2026-11-02 12:26:33 +00:00 · 2026-09-12 21:07:42 -03:00
parent e2dba26bcd
commit 4f06548bc0
3 changed files with 227 additions and 26 deletions
@@ -168,6 +168,27 @@ When submitting multiple projects:
 - Avoid creating duplicate entries within the same README file
 - All entries must meet quality and format requirements independently

+### Audit cleanup pull requests
+
+Keep cleanup batches focused on existing entries and explain each update, move, or removal
+with supporting evidence in the PR description. A cleanup batch may change more than five
+entries or only remove entries. The reviewer identifies cleanup from the diff: every added
+entry line must correspond one-to-one to a removed entry for the same project. Unrelated new
+projects belong in a separate contribution PR; ordinary contributions retain the limit of
+five added entry lines. Non-entry prose or heading changes are outside cleanup scope.
+
+Changed entries still undergo format, section, HTTPS, duplicate, primary-link reachability,
+and repository/documentation checks. Within the same section, cleanup does not reapply the
+new-project activity/archive requirements to an unchanged repository, or require a previously
+repository-less entry to acquire a repository. Replacement repositories and section moves
+receive the normal source and activity checks. These limited exceptions allow legacy links
+to be repaired; they do not establish that the entry meets every current eligibility rule.
+
+Removed entries receive structural checks, without probing the unavailable resources being
+removed. Maintainers must verify the removal evidence, replacement searches, historical
+eligibility, and commercial status as applicable. A passing automated review reports
+`CHECKS PASSED` and requires maintainer review; it does not approve or merge a cleanup.
+
 ## Before Submitting

 1. Search the existing list to make sure the project is not already included.
@@ -176,7 +197,7 @@ When submitting multiple projects:

 ## Required Pull Request Workflows

-Every pull request that adds or updates a README entry must pass both required GitHub Actions
+Every pull request that adds, updates, or removes a README entry must pass both required GitHub Actions
 workflows:

 - **`Validate PR`** checks the added README entries against the parser and formatting rules,
@@ -219,7 +219,11 @@ def analyze_readme_change(
        line for line in substantive_removed if line.strip().startswith("- ")
    ]
    findings: list[Finding] = []
-    if not 1 <= len(entry_lines) <= MAX_README_ENTRIES_PER_PR:
+    previous_entries = match_existing_entries(entry_lines, removed_entry_lines)
+    maintenance = bool(removed_entry_lines) and all(
+        previous is not None for previous in previous_entries
+    )
+    if not maintenance and not 1 <= len(entry_lines) <= MAX_README_ENTRIES_PER_PR:
        findings.append(
            Finding(
                "entry-count",
@@ -234,35 +238,40 @@ def analyze_readme_change(
    unauthorized_removals = [
        line for line in substantive_removed if line not in removed_entry_lines
    ]
-    unmatched_entry_lines = list(entry_lines)
-    unmatched_removal = False
-    for removed_entry_line in removed_entry_lines:
-        matching_index = next(
-            (
-                index
-                for index, entry_line in enumerate(unmatched_entry_lines)
-                if entries_represent_same_project(removed_entry_line, entry_line)
-            ),
-            None,
-        )
-        if matching_index is None:
-            unmatched_removal = True
-            break
-        unmatched_entry_lines.pop(matching_index)
+    unmatched_removal = sum(previous is not None for previous in previous_entries) < len(
+        removed_entry_lines
+    )
    invalid_update = bool(substantive_removed) and (
-        bool(unauthorized_removals) or unmatched_removal
+        bool(unauthorized_removals)
+        or any(ENTRY_RE.match(line) is None for line in removed_entry_lines)
+        or (unmatched_removal and not maintenance)
    )
    if unauthorized_additions or invalid_update:
        findings.append(
            Finding(
                "content",
-                "README changes must add up to five entries or update the same projects "
-                "without other substantive edits",
+                "README changes must add up to five entries, update the same projects, "
+                "or only update/remove existing entries; no other substantive edits",
            )
        )
    return entry_lines, removed_entry_lines, findings


+def match_existing_entries(
+    added: list[str], removed: list[str],
+) -> 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)),
+            None,
+        )
+        previous_entries.append(unmatched.pop(index) if index is not None else None)
+    return previous_entries
+
+
 def remove_entry_lines(readme_text: str, entry_lines: list[str]) -> str:
    lines = readme_text.splitlines()
    for entry_line in entry_lines:
@@ -291,7 +300,9 @@ def readme_changed_lines(
    return added_lines, removed_lines


-def find_entry_section(readme_text: str, entry_line: str) -> str:
+def find_entry_section(
+    readme_text: str, entry_line: str, *, require_unique: bool = True
+) -> str:
    current_section = ""
    matches = 0
    matched_section = ""
@@ -303,6 +314,8 @@ def find_entry_section(readme_text: str, entry_line: str) -> str:
            matches += 1
            matched_section = current_section
    if matches != 1:
+        if not require_unique:
+            return ""
        raise RuntimeError(
            "added README entry could not be located uniquely in the PR head"
        )
@@ -519,6 +532,8 @@ def review_entry(
    other_entry_lines: list[str],
    check_pull_request_duplicates: bool,
    readme_cache: dict[str, str],
+    previous_line: str | None = None,
+    previous_section: str = "",
 ) -> list[Finding]:
    findings: list[Finding] = []
    match = ENTRY_RE.match(line)
@@ -602,8 +617,20 @@ def review_entry(
        )
    github_urls.extend(GITHUB_LINK_RE.findall(line))
    github_urls = list(dict.fromkeys(github_urls))
+    previous_match = ENTRY_RE.match(previous_line or "")
+    previous_github_urls: list[str] = []
+    if previous_match:
+        previous_url = previous_match.group(2).strip()
+        if parse_github_repository_url(previous_url):
+            previous_github_urls.append(previous_url)
+        previous_github_urls.extend(GITHUB_LINK_RE.findall(previous_line or ""))
+    same_section_update = previous_match is not None and previous_section == section
+    unchanged_source = same_section_update and {
+        canonicalize_url(item) for item in github_urls
+    } == {canonicalize_url(item) for item in previous_github_urls}
+
    if not github_urls:
-        if section != "Commercial & Proprietary Services":
+        if section != "Commercial & Proprietary Services" and not unchanged_source:
            findings.append(
                Finding(
                    "github",
@@ -621,10 +648,10 @@ def review_entry(
        else:
            owner, repo_name = repository_parts
            github_repo = client.get_repo(f"{owner}/{repo_name}")
-            if github_repo.archived and not is_historical:
+            if github_repo.archived and not is_historical and not unchanged_source:
                findings.append(Finding("activity", "repository is archived"))
            pushed_at = github_repo.pushed_at
-            if not is_historical and (
+            if not is_historical and not unchanged_source and (
                pushed_at is None
                or pushed_at < current_time - timedelta(days=365)
            ):
@@ -718,7 +745,12 @@ 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)
+    maintenance = bool(removed_entry_lines) and all(
+        previous is not None for previous in previous_entries
+    )
    for index, entry_line in enumerate(entry_lines):
+        previous_line = previous_entries[index] if maintenance else None
        findings.extend(
            review_entry(
                repository,
@@ -731,6 +763,12 @@ def review_pr(
                other_entry_lines=entry_lines[:index] + entry_lines[index + 1 :],
                check_pull_request_duplicates=check_pull_request_duplicates,
                readme_cache=readme_cache,
+                previous_line=previous_line,
+                previous_section=(
+                    find_entry_section(base_readme, previous_line, require_unique=False)
+                    if previous_line
+                    else ""
+                ),
            )
        )

@@ -780,8 +818,8 @@ def main() -> int:
                print("- duplicates: pass (existing README only)")
            else:
                print(f"- {check}: pass")
-        print("Verdict: APPROVE")
-        print("Recommended action: merge")
+        print("Verdict: CHECKS PASSED")
+        print("Recommended action: maintainer review")
        return 0

    print("Findings:")
@@ -1,3 +1,4 @@
+import difflib
 import io
 import unittest
 import warnings
@@ -552,6 +553,139 @@ class PullRequestDuplicateTests(unittest.TestCase):


 class ValidationPipelineTests(unittest.TestCase):
+    def review_cleanup(self, old_entries, new_entries, **kwargs):
+        base = "# awesome-quant\n\n## Trading & Backtesting\n" + old_entries
+        head = "# awesome-quant\n\n## Trading & Backtesting\n" + new_entries
+        patch_text = "".join(difflib.unified_diff(
+            base.splitlines(keepends=True), head.splitlines(keepends=True)
+        ))
+        return self.review(
+            patch_text=patch_text, base_readme=base, head_readme=head, **kwargs
+        )
+
+    def test_cleanup_accepts_six_updates_and_a_removal(self):
+        old = "".join(
+            f"- [Existing {i}](https://github.com/example/existing-{i}) - `Python` - Old description.\n"
+            for i in range(6)
+        )
+        removed = "- [Dead](https://github.com/example/dead) - `Python` - Dead project.\n"
+        self.assertEqual(self.review_cleanup(old + removed, old.replace("Old", "New")), set())
+
+    def test_cleanup_accepts_removal_only_without_live_project_checks(self):
+        old = "- [Dead](https://github.com/example/dead) - `Python` - Dead project.\n"
+        def unavailable(project):
+            project.archived = True
+            project.has_readme = False
+        self.assertEqual(
+            self.review_cleanup(old, "", reachable=False, configure_project=unavailable), set()
+        )
+
+    def test_cleanup_rejects_malformed_removal(self):
+        self.assertIn("content", self.review_cleanup("- malformed entry\n", ""))
+
+    def test_cleanup_rejects_prose_removal(self):
+        old = "- [Dead](https://github.com/example/dead) - `Python` - Dead project.\n"
+        self.assertIn("content", self.review_cleanup(old + "Important policy.\n", ""))
+
+    def test_cleanup_rejects_new_projects_mixed_with_removal(self):
+        old = "- [Dead](https://github.com/example/dead) - `Python` - Dead project.\n"
+        new = "- [Fresh](https://github.com/example/fresh) - `Python` - Fresh project.\n"
+        self.assertIn("content", self.review_cleanup(old, new))
+
+    def test_cleanup_still_checks_changed_entry_fields(self):
+        old = "- [Fresh](https://github.com/example/fresh) - `Python` - Old description.\n"
+        cases = [
+            (old.replace("Old description.", "No period"), "period"),
+            (old.replace("`Python` - ", ""), "tags"),
+            (old.replace("https://", "http://"), "url"),
+        ]
+        for new, expected in cases:
+            with self.subTest(expected=expected):
+                self.assertIn(expected, self.review_cleanup(old, new))
+
+    def test_cleanup_does_not_readmit_unchanged_stale_archived_repository(self):
+        old = "- [Fresh](https://github.com/example/fresh) - `Python` - Old description.\n"
+        def legacy(project):
+            project.archived = True
+            project.pushed_at = NOW - timedelta(days=800)
+        self.assertEqual(
+            self.review_cleanup(old, old.replace("Old", "New"), configure_project=legacy), set()
+        )
+
+    def test_cleanup_still_checks_unchanged_repository_documentation(self):
+        old = "- [Fresh](https://github.com/example/fresh) - `Python` - Old description.\n"
+        def missing_docs(project):
+            project.has_readme = False
+        self.assertIn("documentation", self.review_cleanup(
+            old, old.replace("Old", "New"), configure_project=missing_docs
+        ))
+
+    def test_cleanup_rechecks_replacement_repository_activity(self):
+        old = "- [Fresh](https://github.com/example/old) - `Python` - Old description.\n"
+        def stale(project):
+            project.pushed_at = NOW - timedelta(days=800)
+        self.assertIn("activity", self.review_cleanup(
+            old, old.replace("example/old", "example/fresh"), configure_project=stale
+        ))
+
+    def test_cleanup_section_move_rechecks_activity(self):
+        old = "- [Fresh](https://github.com/example/fresh) - `Python` - Fresh project.\n"
+        def archived(project):
+            project.archived = True
+        self.assertIn("activity", self.review_cleanup(
+            old + "\n## Portfolio Optimization & Risk Analysis\n",
+            "\n## Portfolio Optimization & Risk Analysis\n" + old,
+            configure_project=archived,
+        ))
+
+    def test_cleanup_can_repair_existing_repositoryless_link(self):
+        old = "- [Legacy](https://example.com/docs) - `Python` - Documentation.\n"
+        new = old.replace("/docs)", "/docs/)")
+        self.assertEqual(self.review_cleanup(old, new), set())
+        self.assertIn("reachability", self.review_cleanup(old, new, reachable=False))
+
+    def test_cleanup_cannot_remove_source_from_functional_entry(self):
+        old = "- [Fresh](https://example.com/) - `Python` - Fresh project. [GitHub](https://github.com/example/fresh)\n"
+        new = old.replace(" [GitHub](https://github.com/example/fresh)", "")
+        self.assertIn("github", self.review_cleanup(old, new))
+
+    def test_cleanup_empty_diff_is_not_accepted(self):
+        self.assertIn("entry-count", self.review_cleanup("", ""))
+
+    def test_cleanup_cannot_add_policy_text(self):
+        old = "- [Fresh](https://github.com/example/fresh) - `Python` - Old description.\n"
+        self.assertIn("content", self.review_cleanup(
+            old, old.replace("Old", "New") + "Changed policy.\n"
+        ))
+
+    def test_mixed_new_submission_does_not_get_legacy_activity_exception(self):
+        old = "- [Fresh](https://github.com/example/fresh) - `Python` - Old description.\n"
+        new = old.replace("Old", "New") + "- [Extra](https://github.com/example/extra) - `Python` - Extra project.\n"
+        def stale(project):
+            project.pushed_at = NOW - timedelta(days=800)
+        self.assertIn("activity", self.review_cleanup(old, new, configure_project=stale))
+
+    def test_cleanup_section_move_does_not_grandfather_missing_source(self):
+        old = "- [Legacy](https://example.com/) - `Python` - Legacy project.\n"
+        self.assertIn("github", self.review_cleanup(
+            old + "\n## Portfolio Optimization & Risk Analysis\n",
+            "\n## Portfolio Optimization & Risk Analysis\n" + old,
+        ))
+
+    def test_cleanup_consolidates_duplicate_old_entries(self):
+        old = "- [Fresh](https://github.com/example/fresh) - `Python` - Old description.\n"
+        self.assertEqual(self.review_cleanup(old + old, old.replace("Old", "New")), set())
+
+    def test_cleanup_ambiguous_previous_section_receives_full_checks(self):
+        old = "- [Fresh](https://github.com/example/fresh) - `Python` - Old description.\n"
+        def stale(project):
+            project.pushed_at = NOW - timedelta(days=800)
+        self.assertIn("activity", self.review_cleanup(
+            old + "\n## Portfolio Optimization & Risk Analysis\n" + old,
+            old.replace("Old", "New") + "\n## Portfolio Optimization & Risk Analysis\n",
+            configure_project=stale,
+        ))
+
    def review(
        self,
        *,
@@ -1154,6 +1288,14 @@ class MainTests(unittest.TestCase):
        self.assertIn("description: pass", stdout)
        self.assertIn("duplicates: pass", stdout)

+    def test_success_requires_maintainer_review_including_removal_only(self):
+        for count in (0, 1, 6):
+            with self.subTest(entries=count):
+                result, stdout, _stderr = self.run_main(([], "Audit cleanup", count))
+                self.assertEqual(result, 0)
+                self.assertIn("Recommended action: maintainer review", stdout)
+                self.assertNotIn("Verdict: APPROVE", stdout)
+
    def test_success_reports_actual_entry_count(self):
        changed_files = [
            SimpleNamespace(filename="README.md", patch=FIVE_ENTRY_PATCH)