From 0babeab1085ced2993bce80b0f39f922cb17263f Mon Sep 17 00:00:00 2001 From: Wilson Freitas Date: Sat, 12 Sep 2026 22:16:42 -0300 Subject: [PATCH] Validate identity-preserving repairs to nested GitHub references (#666) --- CONTRIBUTING.md | 8 +- scripts/readme_entries.py | 73 ++++++++++++++ scripts/review_pr.py | 33 ++++++- scripts/validate_readme.py | 28 +++++- tests/test_nested_reference_repairs.py | 130 +++++++++++++++++++++++++ 5 files changed, 265 insertions(+), 7 deletions(-) create mode 100644 tests/test_nested_reference_repairs.py diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1450db7..7679ba1 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -175,7 +175,13 @@ with supporting evidence in the PR description. A cleanup batch may change more 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. +five added entry lines. Existing indented non-entry references may receive a canonical +GitHub repository URL repair only: exactly one HTTPS repository link may change, the old and new links must +resolve to the same immutable GitHub repository ID, and all surrounding text, indentation, +category, parent entry occurrence, and relative position must remain unchanged. Pair each repair with its existing +reference; new references, extra copies, removals, and other non-entry prose or heading +changes are outside cleanup scope. Offline validation checks the repair structure; +`PR Review` additionally verifies repository identity. Changed entries still undergo format, section, HTTPS, duplicate, primary-link reachability, and repository/documentation checks. Within the same section, cleanup does not reapply the diff --git a/scripts/readme_entries.py b/scripts/readme_entries.py index 04ceca7..cf6c628 100644 --- a/scripts/readme_entries.py +++ b/scripts/readme_entries.py @@ -2,6 +2,7 @@ from __future__ import annotations +import difflib import re from dataclasses import dataclass from pathlib import Path @@ -129,3 +130,75 @@ def iter_readme_entries(path: str | Path) -> Iterable[ReadmeEntry]: languages=languages, description=description.strip(), ) + + +def nested_reference_repairs(base: str, head: str) -> list[tuple[int, int, str, str]]: + """Find existing nested references with only one repository URL changed. + + Results contain zero-based old/new line indexes and URLs. This checks + structure only; the authenticated reviewer must verify repository identity. + """ + before, after = base.splitlines(), head.splitlines() + + def contexts(lines: list[str]) -> list[tuple[str, int | None]]: + section = "" + parent = None + result = [] + for index, line in enumerate(lines): + if line.startswith("## "): + section, parent = line, None + elif line.startswith("- "): + parent = index if ENTRY_RE.match(line) else None + result.append((section, parent)) + return result + + old_context, new_context = contexts(before), contexts(after) + operations = difflib.SequenceMatcher( + a=before, b=after, autojunk=False + ).get_opcodes() + unchanged_lines = { + old: new + for operation, a, b, c, d in operations if operation == "equal" + for old, new in zip(range(a, b), range(c, d)) + } + repairs = [] + repository_url = re.compile( + r"https://github\.com/[A-Za-z0-9_-]+/[A-Za-z0-9_.-]+/?" + ) + for operation, a, b, c, d in operations: + if operation != "replace" or b - a != d - c: + continue + for old_index, new_index in zip(range(a, b), range(c, d)): + old, new = before[old_index], after[new_index] + if not re.match(r"^[ \t]+- ", new) or ENTRY_RE.match(new): + continue + old_section, old_parent = old_context[old_index] + new_section, new_parent = new_context[new_index] + if (ENTRY_RE.match(old) or not old_section or old_section != new_section + or old_parent is None or new_parent is None): + continue + # Anchor to the same unchanged parent occurrence, not merely its text. + # A URL repair cannot change the reference's position below that parent. + if (unchanged_lines.get(old_parent) != new_parent + or old_index - old_parent != new_index - new_parent): + continue + old_links = list(MARKDOWN_URL_RE.finditer(old)) + new_links = list(MARKDOWN_URL_RE.finditer(new)) + if len(old_links) != len(new_links): + continue + changed = [ + (a, b) for a, b in zip(old_links, new_links) + if a.group(1) != b.group(1) + ] + if len(changed) != 1: + continue + old_link, new_link = changed[0] + old_url, new_url = old_link.group(1), new_link.group(1) + if not all(repository_url.fullmatch(url) for url in (old_url, new_url)): + continue + if (old[:old_link.start(1)], old[old_link.end(1):]) != ( + new[:new_link.start(1)], new[new_link.end(1):] + ): + continue + repairs.append((old_index, new_index, old_url, new_url)) + return repairs diff --git a/scripts/review_pr.py b/scripts/review_pr.py index 815577f..57a91fe 100644 --- a/scripts/review_pr.py +++ b/scripts/review_pr.py @@ -12,6 +12,7 @@ import re import socket import ssl import sys +from collections import Counter from dataclasses import dataclass from datetime import datetime, timedelta, timezone from functools import cache @@ -33,6 +34,7 @@ from scripts.readme_entries import ( MARKDOWN_URL_RE, VALID_SECTIONS, extract_languages, + nested_reference_repairs, ) @@ -780,8 +782,37 @@ def review_pr( base_readme = read_readme(repository, pull_request.base.sha) head_readme = read_readme(repository, pull_request.head.sha) + patch_text = files[0].patch + verified_lines: Counter[str] = Counter() + reference_repairs = nested_reference_repairs(base_readme, head_readme) + for old_index, new_index, old_url, new_url in reference_repairs: + old_id, new_id = repository_id(old_url), repository_id(new_url) + if old_id is None or old_id != new_id: + findings.append(Finding( + "reference", "nested reference must retain its verified GitHub repository identity" + )) + continue + verified_lines["-" + base_readme.splitlines()[old_index]] += 1 + verified_lines["+" + head_readme.splitlines()[new_index]] += 1 + if verified_lines: + remaining_patch = [] + for line in (patch_text or "").splitlines(): + if verified_lines[line]: + verified_lines[line] -= 1 + else: + remaining_patch.append(line) + patch_text = "\n".join(remaining_patch) + if any(verified_lines.values()): + findings.append(Finding( + "reference", "nested reference repair missing from PR patch" + )) + elif not any( + line.startswith(("+", "-")) and not line.startswith(("+++", "---")) + for line in remaining_patch + ): + return findings, pull_request.title, 0 entry_lines, removed_entry_lines, change_findings = analyze_readme_change( - files[0].patch, repository_id, + patch_text, repository_id, ) findings.extend(change_findings) if not entry_lines: diff --git a/scripts/validate_readme.py b/scripts/validate_readme.py index db0c239..918c30e 100755 --- a/scripts/validate_readme.py +++ b/scripts/validate_readme.py @@ -32,6 +32,7 @@ from scripts.readme_entries import ( ReadmeEntry, extract_languages, iter_readme_entries, + nested_reference_repairs, ) @@ -255,11 +256,15 @@ def validate_entry( def validate_malformed_added_lines( - readme_path: Path, added_lines: set[int] + readme_path: Path, added_lines: set[int], *, base_readme: str = "" ) -> list[Issue]: issues: list[Issue] = [] - lines = readme_path.read_text(encoding="utf-8").splitlines() - for line_number in sorted(added_lines): + head_readme = readme_path.read_text(encoding="utf-8") + lines = head_readme.splitlines() + repaired_lines = { + new + 1 for _, new, _, _ in nested_reference_repairs(base_readme, head_readme) + } + for line_number in sorted(added_lines - repaired_lines): if line_number < 1 or line_number > len(lines): continue line = lines[line_number - 1] @@ -315,10 +320,21 @@ def main() -> int: duplicate_names, duplicate_urls = build_duplicate_indexes(entries) added_lines: set[int] | None = None + base_readme = "" if args.diff_from: try: added_lines = read_added_line_numbers(args.diff_from, readme_path) - except RuntimeError as exc: + merge_base = subprocess.check_output( + ["git", "merge-base", args.diff_from, "HEAD"], text=True + ).strip() + root = subprocess.check_output( + ["git", "rev-parse", "--show-toplevel"], text=True + ).strip() + relative_path = readme_path.resolve().relative_to(Path(root)) + base_readme = subprocess.check_output( + ["git", "show", f"{merge_base}:{relative_path}"], text=True + ) + except (RuntimeError, subprocess.CalledProcessError, ValueError) as exc: print(f"ERROR {exc}", file=sys.stderr) return 2 selected_entries = [ @@ -335,7 +351,9 @@ def main() -> int: issues: list[Issue] = [] if added_lines is not None: - issues.extend(validate_malformed_added_lines(readme_path, added_lines)) + issues.extend(validate_malformed_added_lines( + readme_path, added_lines, base_readme=base_readme + )) for entry in selected_entries: issues.extend( diff --git a/tests/test_nested_reference_repairs.py b/tests/test_nested_reference_repairs.py new file mode 100644 index 0000000..3ec6694 --- /dev/null +++ b/tests/test_nested_reference_repairs.py @@ -0,0 +1,130 @@ +import difflib +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path +from unittest.mock import patch + +from github import GithubException + +from scripts.readme_entries import iter_readme_entries, nested_reference_repairs +from scripts.validate_readme import validate_malformed_added_lines +from tests import test_review_pr as fixtures + +OLD = ( + ' - Library - Python [package](https://pypi.org/project/demo/) ' + 'and [C++](https://github.com/example/old)' +) +NEW = OLD.replace('/old)', '/new)') +PARENT = '- [Parent](https://github.com/example/parent) - `Python` - Toolkit.\n' +BASE = '## Trading & Backtesting\n' + PARENT + OLD + '\n' + + +class NestedReferenceRepairTests(unittest.TestCase): + def review(self, head, *, base=BASE, identity=123): + original = fixtures.FakeClient.get_repo + def lookup(client, name): + if identity is None and name == 'example/old': + raise GithubException(404, {'message': 'Not Found'}) + repo = original(client, name) + if name.startswith('example/'): + repo.id = 123 if name.endswith('/old') else identity + return repo + patch_text = ''.join(difflib.unified_diff( + base.splitlines(keepends=True), head.splitlines(keepends=True) + )) + with patch.object(fixtures.FakeClient, 'get_repo', lookup): + return fixtures.ValidationPipelineTests().review( + patch_text=patch_text, base_readme=base, head_readme=head + ) + + def test_existing_reference_only_repair(self): + self.assertEqual(self.review(BASE.replace(OLD, NEW)), set()) + + def test_repair_mixed_with_entry_update(self): + base = BASE + PARENT.replace('[Parent]', '[Other]').replace('/parent)', '/other)') + head = base.replace(OLD, NEW).replace('[Other]', '[Renamed]') + self.assertEqual(self.review(head, base=base), set()) + + def test_new_changed_moved_or_duplicated_references_rejected(self): + cases = [ + BASE + NEW + '\n', + BASE.replace(OLD, NEW + '\n' + NEW), + BASE.replace(OLD, NEW.replace('Library -', 'Different -')), + BASE.replace(OLD, NEW.replace(' -', ' -')), + BASE.replace(OLD, NEW.lstrip()), + BASE.replace(OLD, NEW.replace('[C++]', '[New label]')), + BASE.replace(OLD, NEW.replace('/project/demo/', '/project/other/')), + (BASE.replace(OLD, '') + '## Market Data & Data Sources\n' + + PARENT.replace('[Parent]', '[Moved]') + NEW + '\n'), + BASE.replace(OLD, '') + PARENT.replace('[Parent]', '[Other]') + NEW + '\n', + BASE.replace(OLD + '\n', ''), + ] + for head in cases: + with self.subTest(head=head): + self.assertTrue(self.review(head)) + + def test_reference_cannot_move_between_identical_parent_occurrences(self): + base = BASE + PARENT + head = BASE.replace(OLD + '\n', '') + PARENT + NEW + '\n' + self.assertEqual(nested_reference_repairs(base, head), []) + + def test_reference_cannot_reorder_within_parent(self): + other = OLD.replace('Library -', 'Other -') + base = BASE + other + '\n' + head = BASE.replace(OLD, other) + NEW + '\n' + self.assertEqual(nested_reference_repairs(base, head), []) + + def test_replacement_or_unverified_repository_rejected(self): + for identity in (456, None): + with self.subTest(identity=identity): + self.assertTrue(self.review(BASE.replace(OLD, NEW), identity=identity)) + + def test_non_repository_destinations_rejected(self): + for url in ('http://github.com/example/new', 'https://user@github.com/example/new', + 'https://other.example/example/new', 'https://github.com/example/new?q=x', + 'https://github.com/example/new#x', 'https://github.com/example/new/tree/main'): + with self.subTest(url=url): + self.assertTrue(self.review(BASE.replace('https://github.com/example/old', url))) + + def test_offline_validation_preserves_entry_count(self): + with tempfile.TemporaryDirectory() as directory: + readme = Path(directory) / 'README.md' + readme.write_text(BASE.replace(OLD, NEW)) + self.assertEqual(len(list(iter_readme_entries(readme))), 1) + self.assertEqual(validate_malformed_added_lines(readme, {3}, base_readme=BASE), []) + self.assertTrue(validate_malformed_added_lines( + readme, {3}, base_readme=BASE.replace(OLD, '') + )) + + def test_offline_cli_uses_merge_base_for_existing_reference(self): + script = Path(__file__).resolve().parents[1] / 'scripts' / 'validate_readme.py' + with tempfile.TemporaryDirectory() as directory: + def git(*args): + return subprocess.run(['git', *args], cwd=directory, check=True, + capture_output=True, text=True).stdout.strip() + git('init', '-q') + git('config', 'user.email', 'test@example.com') + git('config', 'user.name', 'Test') + readme = Path(directory) / 'README.md' + readme.write_text(BASE) + git('add', 'README.md') + git('commit', '-qm', 'Base') + base = git('rev-parse', 'HEAD') + readme.write_text(BASE.replace(OLD, NEW)) + git('commit', '-qam', 'Repair') + result = subprocess.run( + [sys.executable, str(script), '--diff-from', base], cwd=directory, + capture_output=True, text=True, + ) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + self.assertIn('Validated 0 entries', result.stdout) + readme.write_text(BASE.replace(OLD, NEW) + NEW + '\n') + git('commit', '-qam', 'Extra reference') + result = subprocess.run( + [sys.executable, str(script), '--diff-from', base], cwd=directory, + capture_output=True, text=True, + ) + self.assertEqual(result.returncode, 1, result.stdout + result.stderr) + self.assertIn('added README bullet does not match entry regex', result.stdout)