mirror of
https://github.com/wilsonfreitas/awesome-quant.git
synced 2026-11-02 12:26:33 +00:00
Validate identity-preserving repairs to nested GitHub references
This commit is contained in:
+7
-1
@@ -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
|
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
|
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
|
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,
|
Changed entries still undergo format, section, HTTPS, duplicate, primary-link reachability,
|
||||||
and repository/documentation checks. Within the same section, cleanup does not reapply the
|
and repository/documentation checks. Within the same section, cleanup does not reapply the
|
||||||
|
|||||||
@@ -2,6 +2,7 @@
|
|||||||
|
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import difflib
|
||||||
import re
|
import re
|
||||||
from dataclasses import dataclass
|
from dataclasses import dataclass
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
@@ -129,3 +130,75 @@ def iter_readme_entries(path: str | Path) -> Iterable[ReadmeEntry]:
|
|||||||
languages=languages,
|
languages=languages,
|
||||||
description=description.strip(),
|
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
|
||||||
|
|||||||
+32
-1
@@ -12,6 +12,7 @@ import re
|
|||||||
import socket
|
import socket
|
||||||
import ssl
|
import ssl
|
||||||
import sys
|
import sys
|
||||||
|
from collections import Counter
|
||||||
from dataclasses import dataclass
|
from dataclasses import dataclass
|
||||||
from datetime import datetime, timedelta, timezone
|
from datetime import datetime, timedelta, timezone
|
||||||
from functools import cache
|
from functools import cache
|
||||||
@@ -33,6 +34,7 @@ from scripts.readme_entries import (
|
|||||||
MARKDOWN_URL_RE,
|
MARKDOWN_URL_RE,
|
||||||
VALID_SECTIONS,
|
VALID_SECTIONS,
|
||||||
extract_languages,
|
extract_languages,
|
||||||
|
nested_reference_repairs,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
@@ -780,8 +782,37 @@ def review_pr(
|
|||||||
|
|
||||||
base_readme = read_readme(repository, pull_request.base.sha)
|
base_readme = read_readme(repository, pull_request.base.sha)
|
||||||
head_readme = read_readme(repository, pull_request.head.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(
|
entry_lines, removed_entry_lines, change_findings = analyze_readme_change(
|
||||||
files[0].patch, repository_id,
|
patch_text, repository_id,
|
||||||
)
|
)
|
||||||
findings.extend(change_findings)
|
findings.extend(change_findings)
|
||||||
if not entry_lines:
|
if not entry_lines:
|
||||||
|
|||||||
@@ -32,6 +32,7 @@ from scripts.readme_entries import (
|
|||||||
ReadmeEntry,
|
ReadmeEntry,
|
||||||
extract_languages,
|
extract_languages,
|
||||||
iter_readme_entries,
|
iter_readme_entries,
|
||||||
|
nested_reference_repairs,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
@@ -255,11 +256,15 @@ def validate_entry(
|
|||||||
|
|
||||||
|
|
||||||
def validate_malformed_added_lines(
|
def validate_malformed_added_lines(
|
||||||
readme_path: Path, added_lines: set[int]
|
readme_path: Path, added_lines: set[int], *, base_readme: str = ""
|
||||||
) -> list[Issue]:
|
) -> list[Issue]:
|
||||||
issues: list[Issue] = []
|
issues: list[Issue] = []
|
||||||
lines = readme_path.read_text(encoding="utf-8").splitlines()
|
head_readme = readme_path.read_text(encoding="utf-8")
|
||||||
for line_number in sorted(added_lines):
|
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):
|
if line_number < 1 or line_number > len(lines):
|
||||||
continue
|
continue
|
||||||
line = lines[line_number - 1]
|
line = lines[line_number - 1]
|
||||||
@@ -315,10 +320,21 @@ def main() -> int:
|
|||||||
duplicate_names, duplicate_urls = build_duplicate_indexes(entries)
|
duplicate_names, duplicate_urls = build_duplicate_indexes(entries)
|
||||||
|
|
||||||
added_lines: set[int] | None = None
|
added_lines: set[int] | None = None
|
||||||
|
base_readme = ""
|
||||||
if args.diff_from:
|
if args.diff_from:
|
||||||
try:
|
try:
|
||||||
added_lines = read_added_line_numbers(args.diff_from, readme_path)
|
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)
|
print(f"ERROR {exc}", file=sys.stderr)
|
||||||
return 2
|
return 2
|
||||||
selected_entries = [
|
selected_entries = [
|
||||||
@@ -335,7 +351,9 @@ def main() -> int:
|
|||||||
|
|
||||||
issues: list[Issue] = []
|
issues: list[Issue] = []
|
||||||
if added_lines is not None:
|
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:
|
for entry in selected_entries:
|
||||||
issues.extend(
|
issues.extend(
|
||||||
|
|||||||
@@ -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)
|
||||||
Reference in New Issue
Block a user