diff --git a/.github/workflows/handle_potential_conflicts.py b/.github/workflows/handle_potential_conflicts.py index 2acec3e56727..5d93ed59d02f 100755 --- a/.github/workflows/handle_potential_conflicts.py +++ b/.github/workflows/handle_potential_conflicts.py @@ -4,14 +4,14 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """ -Validate candidate PR conflicts and maintain advisory PR comments. +Find open PRs that conflict with each other and maintain advisory PR comments. Usage: - $ ./handle_potential_conflicts.py --pr-number + $ ./handle_potential_conflicts.py [--dry-run] -The overlap pass only finds same-base PRs that touch the same files. This -script checks whether each candidate PR can still merge after the current PR is -pre-merged, then updates one managed comment per affected PR. +Each run checks every pair of open, ready-for-review PRs that share a base +branch and touch the same files, then rewrites one managed comment per PR from +the results. A pair the run cannot check keeps what the comments already say. """ from __future__ import annotations @@ -26,6 +26,7 @@ import sys import urllib.request from dataclasses import dataclass +from datetime import datetime, timedelta, timezone from typing import Any from urllib.parse import quote @@ -35,15 +36,17 @@ COMMENT_END = "-->" TRUSTED_COMMENT_AUTHORS = {"github-actions[bot]"} MAX_FILE_DETAILS = 12 +# How long a PR that was closed or made a draft keeps being checked for a comment to delete. +CLEANUP_WINDOW = timedelta(days=1) # Error statuses, network errors and timeouts are OSError; a malformed body is ValueError. REQUEST_ERRORS = (OSError, http.client.HTTPException, ValueError) @dataclass class ManagedComment: - comment_id: int | None + comment_id: int body: str - state: dict[str, Any] + listed_prs: set[int] def github_headers() -> dict[str, str]: @@ -119,85 +122,53 @@ def github_delete(path: str) -> None: http_request("DELETE", api_url(path), github_headers()) -def normalize_state_item(item: Any) -> dict[str, Any] | None: - if not isinstance(item, dict): - return None - - try: - number = int(item["number"]) - except (KeyError, TypeError, ValueError): - return None - - files = item.get("files", []) - if not isinstance(files, list): - files = [] - - return { - "number": number, - "title": " ".join(str(item.get("title", "Unknown title")).split()), - "url": str(item.get("url") or f"https://github.com/{repo_name()}/pull/{number}"), - "files": [str(filename) for filename in files], - } - - -def normalize_state(state: Any) -> dict[str, Any]: +def listed_prs_from_state(state: Any) -> set[int]: if not isinstance(state, dict): - state = {} - - outbound = state.get("outbound", []) - inbound = state.get("inbound", {}) - - if not isinstance(outbound, list): - outbound = [] - if not isinstance(inbound, dict): - inbound = {} - - normalized_outbound = [] - for item in outbound: - normalized_item = normalize_state_item(item) - if normalized_item is not None: - normalized_outbound.append(normalized_item) - - normalized_inbound = {} - for key, value in inbound.items(): - normalized_item = normalize_state_item(value) - if normalized_item is not None: - normalized_inbound[str(key)] = normalized_item + return set() - return { - "outbound": normalized_outbound, - "inbound": normalized_inbound, - } + if "conflicts" in state: + entries = state["conflicts"] + else: + # Comments written when each PR event ran its own check stored both merge orders. + outbound = state.get("outbound") + inbound = state.get("inbound") + entries = [ + *(outbound if isinstance(outbound, list) else []), + *(inbound.values() if isinstance(inbound, dict) else []), + ] + + numbers = set() + for entry in entries if isinstance(entries, list) else []: + try: + numbers.add(int(entry["number"] if isinstance(entry, dict) else entry)) + except (KeyError, TypeError, ValueError): + continue + return numbers -def extract_state(body: str) -> dict[str, Any]: +def extract_listed_prs(body: str) -> set[int]: # The state block is rendered last, but comments written before that change # start with it. Anchor on the last marker unless the body opens with one, # so marker-like text in advisory content cannot shadow the real block. marker_index = 0 if body.startswith(COMMENT_START) else body.rfind(COMMENT_START) if marker_index == -1: - return normalize_state({}) + return set() json_start = body.find("\n", marker_index) marker_end = body.find(COMMENT_END, marker_index) if json_start == -1 or marker_end == -1 or json_start > marker_end: - return normalize_state({}) + return set() encoded_state = body[json_start:marker_end].strip() if not encoded_state: - return normalize_state({}) + return set() try: raw_state = base64.b64decode(encoded_state).decode("utf8") - return normalize_state(json.loads(raw_state)) - except (ValueError, json.JSONDecodeError): + return listed_prs_from_state(json.loads(raw_state)) + except ValueError: print("Warning: managed conflict comment has invalid state; replacing it", file=sys.stderr) - return normalize_state({}) - - -def state_is_empty(state: dict[str, Any]) -> bool: - normalized = normalize_state(state) - return not normalized["outbound"] and not normalized["inbound"] + return set() def compact_title(title: str) -> str: @@ -233,36 +204,18 @@ def format_pr_line(item: dict[str, Any]) -> str: return f"- [#{number}: {title}]({url}){format_files(files)}" -def render_comment_body(state: dict[str, Any]) -> str: - state = normalize_state(state) - state_json = json.dumps(state, sort_keys=True, separators=(",", ":")) +def render_comment_body(conflicts: list[dict[str, Any]]) -> str: + conflicts = sorted(conflicts, key=lambda item: int(item["number"])) + state_json = json.dumps({"conflicts": [int(item["number"]) for item in conflicts]}, separators=(",", ":")) state_encoded = base64.b64encode(state_json.encode("utf8")).decode("ascii") lines = [ "## Potential PR merge conflicts", "", - "This is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order.", + "This is advisory only. It does not block CI. This PR and each open PR below change the same files and " + "cannot be merged together cleanly, so whichever of the two merges second will likely need a rebase:", + "", ] - - if state["outbound"]: - lines.extend([ - "", - "### If this PR merges first", - "", - "These open PRs will likely need a rebase:", - "", - ]) - lines.extend(format_pr_line(item) for item in sorted(state["outbound"], key=lambda item: int(item["number"]))) - - if state["inbound"]: - lines.extend([ - "", - "### If these PRs merge first", - "", - "This PR will likely need a rebase:", - "", - ]) - inbound_items = sorted(state["inbound"].values(), key=lambda item: int(item["number"])) - lines.extend(format_pr_line(item) for item in inbound_items) + lines.extend(format_pr_line(item) for item in conflicts) # The state block goes last so notification previews start with the advisory text. lines.extend([ @@ -272,15 +225,7 @@ def render_comment_body(state: dict[str, Any]) -> str: COMMENT_END, ]) - return "\n".join(lines).rstrip() + "\n" - - -def get_pr_json(pr_num: int) -> dict[str, Any] | None: - try: - return github_get(f"pulls/{pr_num}") - except REQUEST_ERRORS as e: - print(f"Warning: Error fetching PR {pr_num}: {e}", file=sys.stderr) - return None + return "\n".join(lines) + "\n" def list_issue_comments(pr_num: int) -> list[dict[str, Any]]: @@ -304,51 +249,30 @@ def find_managed_comments(pr_num: int) -> list[ManagedComment]: body = comment.get("body") or "" if COMMENT_START in body: managed_comments.append(ManagedComment( - comment_id=comment.get("id"), + comment_id=comment["id"], body=body, - state=extract_state(body), + listed_prs=extract_listed_prs(body), )) return managed_comments -def select_managed_comment(managed_comments: list[ManagedComment]) -> ManagedComment: - if managed_comments: - return managed_comments[-1] - return ManagedComment(comment_id=None, body="", state=normalize_state({})) - - -def get_managed_comment(pr_num: int) -> ManagedComment: - return select_managed_comment(find_managed_comments(pr_num)) - - -def save_managed_comment(pr_num: int, state: dict[str, Any]) -> None: - managed_comments = find_managed_comments(pr_num) - managed_comment = select_managed_comment(managed_comments) - state = normalize_state(state) - - if state_is_empty(state): - for comment in managed_comments: - if comment.comment_id is not None: - github_delete(f"issues/comments/{comment.comment_id}") - if managed_comments: - print(f"Deleted stale conflict comments on PR #{pr_num}", file=sys.stderr) - return - - body = render_comment_body(state) - if managed_comment.comment_id is None: - github_post(f"issues/{pr_num}/comments", {"body": body}) - print(f"Created conflict comment on PR #{pr_num}", file=sys.stderr) - elif managed_comment.body != body: - github_patch(f"issues/comments/{managed_comment.comment_id}", {"body": body}) - print(f"Updated conflict comment on PR #{pr_num}", file=sys.stderr) - else: - print(f"Conflict comment on PR #{pr_num} is already up to date", file=sys.stderr) +def save_managed_comment(pr_num: int, managed_comments: list[ManagedComment], conflicts: list[dict[str, Any]]) -> None: + """Make the PR's managed comment list `conflicts`, or delete it if there are none.""" + kept = managed_comments[-1] if conflicts and managed_comments else None + if conflicts: + body = render_comment_body(conflicts) + if kept is None: + github_post(f"issues/{pr_num}/comments", {"body": body}) + print(f"Created conflict comment on PR #{pr_num}", file=sys.stderr) + elif kept.body != body: + github_patch(f"issues/comments/{kept.comment_id}", {"body": body}) + print(f"Updated conflict comment on PR #{pr_num}", file=sys.stderr) for comment in managed_comments: - if comment.comment_id is not None and comment.comment_id != managed_comment.comment_id: + if comment is not kept: github_delete(f"issues/comments/{comment.comment_id}") - print(f"Deleted duplicate conflict comment on PR #{pr_num}", file=sys.stderr) + print(f"Deleted {'duplicate' if conflicts else 'stale'} conflict comment on PR #{pr_num}", file=sys.stderr) def build_pr_item(pr_json: dict[str, Any], files: list[str]) -> dict[str, Any]: @@ -360,13 +284,6 @@ def build_pr_item(pr_json: dict[str, Any], files: list[str]) -> dict[str, Any]: } -def item_number(item: dict[str, Any]) -> int | None: - try: - return int(item["number"]) - except (KeyError, TypeError, ValueError): - return None - - def file_names_from_pr_file(file_info: dict[str, Any]) -> set[str]: names = set() @@ -381,45 +298,25 @@ def file_names_from_pr_file(file_info: dict[str, Any]) -> set[str]: return names -def get_pr_files(pr_num: int) -> set[str]: +def get_pr_files(pr_num: int) -> set[str] | None: files = set() - for file_info in github_get_paginated(f"pulls/{pr_num}/files?per_page=100"): - files.update(file_names_from_pr_file(file_info)) + try: + for file_info in github_get_paginated(f"pulls/{pr_num}/files?per_page=100"): + files.update(file_names_from_pr_file(file_info)) + except REQUEST_ERRORS as e: + print(f"Warning: Error fetching the files of PR {pr_num}: {e}", file=sys.stderr) + return None return files -def discover_conflict_candidates(our_pr_json: dict[str, Any]) -> list[dict[str, Any]]: - """Find same-base open PRs that touch at least one of the same files.""" - our_pr_num = int(our_pr_json["number"]) - our_base = our_pr_json["base"]["ref"] - our_files = get_pr_files(our_pr_num) - - if not our_files: - print(f"PR #{our_pr_num} has no changed files", file=sys.stderr) - return [] - - candidates = [] - open_prs = github_get_paginated(f"pulls?state=open&base={quote(our_base, safe='')}&per_page=100") - for pr_json in open_prs: - conflict_pr_num = int(pr_json["number"]) - if conflict_pr_num == our_pr_num: - continue - if pr_json.get("draft", False): - print(f"PR #{conflict_pr_num} is a draft. Skipping conflict check", file=sys.stderr) - continue - - conflict_files = get_pr_files(conflict_pr_num) - overlap = sorted(our_files & conflict_files) - if not overlap: - continue - - candidates.append({ - "number": conflict_pr_num, - "files": sorted(conflict_files), - "conflicts": overlap, - }) - - return candidates +def pr_needs_rebase(pr_num: int) -> bool | None: + """Return None if the PR could not be fetched or GitHub has not yet checked it against its base.""" + try: + mergeable_state = github_get(f"pulls/{pr_num}").get("mergeable_state") + except REQUEST_ERRORS as e: + print(f"Warning: Error fetching PR {pr_num}: {e}", file=sys.stderr) + return None + return None if mergeable_state == "unknown" else mergeable_state == "dirty" def branches_can_merge(our_pr_label: str, conflict_pr_label: str) -> bool | None: @@ -444,193 +341,106 @@ def branches_can_merge(our_pr_label: str, conflict_pr_label: str) -> bool | None return None -def validate_conflicts(candidates: list[dict[str, Any]], our_pr_json: dict[str, Any]) -> list[dict[str, Any]]: - our_pr_label = our_pr_json["head"]["label"] - validated_conflicts = [] - - for conflict in candidates: - if "number" not in conflict: - print("Warning: Skipping conflict entry without 'number' field", file=sys.stderr) - continue - - conflict_pr_num = int(conflict["number"]) - conflict_pr_json = get_pr_json(conflict_pr_num) - if conflict_pr_json is None: - print(f"Warning: Failed to fetch PR {conflict_pr_num}, skipping", file=sys.stderr) - continue - - if "head" not in conflict_pr_json or "label" not in conflict_pr_json["head"]: - print(f"Warning: Invalid PR data structure for PR {conflict_pr_num}, skipping", file=sys.stderr) - continue - - if conflict_pr_json.get("mergeable_state") == "dirty": - print(f"PR #{conflict_pr_num} already needs rebase. Skipping conflict check", file=sys.stderr) - continue - - if conflict_pr_json.get("draft", False): - print(f"PR #{conflict_pr_num} is a draft. Skipping conflict check", file=sys.stderr) - continue - - can_merge = branches_can_merge(our_pr_label, conflict_pr_json["head"]["label"]) - if can_merge is True: - print(f"PR #{conflict_pr_num} still merges after PR #{our_pr_json['number']}", file=sys.stderr) - continue - if can_merge is None: - continue - - files = conflict.get("conflicts", []) - item = build_pr_item(conflict_pr_json, files if isinstance(files, list) else []) - validated_conflicts.append(item) - - return validated_conflicts - - -def update_comments(our_pr_json: dict[str, Any], validated_conflicts: list[dict[str, Any]]) -> None: - our_pr_num = int(our_pr_json["number"]) - our_managed_comment = get_managed_comment(our_pr_num) - previous_outbound_numbers = { - number - for item in our_managed_comment.state["outbound"] - if isinstance(item, dict) - for number in [item_number(item)] - if number is not None - } - current_outbound_numbers = {int(item["number"]) for item in validated_conflicts} - - our_state = normalize_state(our_managed_comment.state) - our_state["outbound"] = validated_conflicts - save_managed_comment(our_pr_num, our_state) - - targets_to_update = previous_outbound_numbers | current_outbound_numbers - our_item_by_target = { - int(item["number"]): build_pr_item(our_pr_json, [str(filename) for filename in item.get("files", [])]) - for item in validated_conflicts - } - - for target_pr_num in sorted(targets_to_update): - target_comment = get_managed_comment(target_pr_num) - target_state = normalize_state(target_comment.state) - inbound = target_state["inbound"] - - if target_pr_num in our_item_by_target: - inbound[str(our_pr_num)] = our_item_by_target[target_pr_num] - else: - inbound.pop(str(our_pr_num), None) - - target_state["inbound"] = inbound - save_managed_comment(target_pr_num, target_state) - - -def remove_outbound_reference(state: dict[str, Any], pr_num: int) -> dict[str, Any]: - state = normalize_state(state) - outbound = [] - - for item in state["outbound"]: - if not isinstance(item, dict): - continue - if item_number(item) == pr_num: - continue - outbound.append(item) - - state["outbound"] = outbound - return state - - -def cleanup_closed_pr_comments(pr_num: int) -> None: - managed_comment = get_managed_comment(pr_num) - state = normalize_state(managed_comment.state) - outbound_numbers = { - number - for item in state["outbound"] - if isinstance(item, dict) - for number in [item_number(item)] - if number is not None - } - inbound_numbers = { - int(number) - for number in state["inbound"] - if str(number).isdigit() - } - - for target_pr_num in sorted(outbound_numbers): - target_comment = get_managed_comment(target_pr_num) - target_state = normalize_state(target_comment.state) - target_state["inbound"].pop(str(pr_num), None) - save_managed_comment(target_pr_num, target_state) - - for source_pr_num in sorted(inbound_numbers): - source_comment = get_managed_comment(source_pr_num) - source_state = remove_outbound_reference(source_comment.state, pr_num) - save_managed_comment(source_pr_num, source_state) - - deleted_comment = False - for comment in find_managed_comments(pr_num): - if comment.comment_id is not None: - github_delete(f"issues/comments/{comment.comment_id}") - deleted_comment = True - if deleted_comment: - print(f"Deleted conflict comment on closed PR #{pr_num}", file=sys.stderr) +def find_conflicts(prs: list[dict[str, Any]]) -> tuple[dict[int, list[dict[str, Any]]], dict[int, dict[int, dict[str, Any]]]]: + """Return the PRs each PR conflicts with, and the PRs each one overlaps but could not be checked against.""" + files = {int(pr["number"]): get_pr_files(int(pr["number"])) for pr in prs} + conflicts: dict[int, list[dict[str, Any]]] = {number: [] for number in files} + unchecked: dict[int, dict[int, dict[str, Any]]] = {number: {} for number in files} + needs_rebase: dict[int, bool | None] = {} + + for index, pr in enumerate(prs): + for other in prs[index + 1:]: + if pr["base"]["ref"] != other["base"]["ref"]: + continue + a, b = int(pr["number"]), int(other["number"]) + files_a, files_b = files[a], files[b] + if files_a is None or files_b is None: + # Without both file lists the overlap is unknown, so the pair stays unchecked. + overlap = [] + else: + overlap = sorted(files_a & files_b) + if not overlap: + continue + + can_merge = None + if overlap: + for number in (a, b): + if number not in needs_rebase: + needs_rebase[number] = pr_needs_rebase(number) + # A PR that already needs a rebase is checked again once it is rebased. + if needs_rebase[a] is False and needs_rebase[b] is False: + # A merge conflict is symmetric, so one check covers both merge orders. + can_merge = branches_can_merge(pr["head"]["label"], other["head"]["label"]) + + if can_merge is None: + unchecked[a][b] = build_pr_item(other, overlap) + unchecked[b][a] = build_pr_item(pr, overlap) + elif not can_merge: + conflicts[a].append(build_pr_item(other, overlap)) + conflicts[b].append(build_pr_item(pr, overlap)) + + return conflicts, unchecked + + +def recently_updated(pr: dict[str, Any], since: datetime) -> bool: + return datetime.fromisoformat(pr["updated_at"].replace("Z", "+00:00")) >= since def parse_args() -> argparse.Namespace: parser = argparse.ArgumentParser( - description="Validate potential PR conflicts and maintain advisory comments.", - ) - parser.add_argument( - "--pr-number", - type=int, - required=True, - help="Pull request number to inspect.", + description="Find conflicting open PRs and maintain advisory comments.", ) parser.add_argument( "--dry-run", action="store_true", - help="Print the current PR comment body instead of creating, updating, or deleting comments.", - ) - parser.add_argument( - "--cleanup-closed", - action="store_true", - help="Remove advisory state for a PR that has been closed.", + help="Print the comment bodies instead of creating, updating, or deleting comments.", ) return parser.parse_args() def main() -> None: args = parse_args() - our_pr_num = args.pr_number - - if args.cleanup_closed: + open_prs = github_get_paginated("pulls?state=open&per_page=100") + ready_prs = [pr for pr in open_prs if not pr.get("draft", False)] + # Closing a PR or making it a draft updates it, so this finds every PR that left the + # checked set lately, even if no comment lists it or an earlier run failed to clean it. + since = datetime.now(timezone.utc) - CLEANUP_WINDOW + closed_prs = github_get("pulls?state=closed&sort=updated&direction=desc&per_page=100") + left_prs = [pr for pr in open_prs if pr.get("draft", False)] + closed_prs + recently_left = {int(pr["number"]) for pr in left_prs if recently_updated(pr, since)} + conflicts, unchecked = find_conflicts(ready_prs) + + failed = [] + previously_listed = set() + for number, items in sorted(conflicts.items()): + try: + managed_comments = find_managed_comments(number) + listed = set().union(*(comment.listed_prs for comment in managed_comments)) + previously_listed |= listed + # Keep what the comment already says about PRs this run could not check. + items = items + [item for other, item in unchecked[number].items() if other in listed] + if args.dry_run: + print(f"PR #{number}:\n{render_comment_body(items) if items else 'no comment'}\n") + else: + save_managed_comment(number, managed_comments, items) + except REQUEST_ERRORS as e: + print(f"Warning: could not update the conflict comment on PR #{number}: {e}", file=sys.stderr) + failed.append(number) + + # Drafts and closed PRs are not checked; clear any comment they still have + # from when they were open and ready for review. + for number in sorted((previously_listed | recently_left) - conflicts.keys()): if args.dry_run: - print(f"Would clean advisory conflict comments for closed PR #{our_pr_num}") - else: - cleanup_closed_pr_comments(our_pr_num) - return - - our_pr_json = get_pr_json(our_pr_num) - - if our_pr_json is None: - print(f"Error: Failed to fetch PR {our_pr_num}", file=sys.stderr) - sys.exit(1) - - if "head" not in our_pr_json or "label" not in our_pr_json["head"]: - print(f"Error: Invalid PR data structure for PR {our_pr_num}", file=sys.stderr) - sys.exit(1) - - if our_pr_json.get("draft", False): - if args.dry_run: - print(f"Would clean advisory conflict comments for draft PR #{our_pr_num}") - else: - cleanup_closed_pr_comments(our_pr_num) - return - - candidates = discover_conflict_candidates(our_pr_json) - validated_conflicts = validate_conflicts(candidates, our_pr_json) - print(f"Conflicting PRs after validation: {[item['number'] for item in validated_conflicts]}", file=sys.stderr) - - if args.dry_run: - print(render_comment_body({"outbound": validated_conflicts, "inbound": {}})) - else: - update_comments(our_pr_json, validated_conflicts) + print(f"PR #{number}: delete any conflict comment\n") + continue + try: + save_managed_comment(number, find_managed_comments(number), []) + except REQUEST_ERRORS as e: + print(f"Warning: could not delete the conflict comment on PR #{number}: {e}", file=sys.stderr) + failed.append(number) + + if failed: + sys.exit(f"Error: could not update conflict comments on PRs {failed}") if __name__ == "__main__": diff --git a/.github/workflows/predict-conflicts.yml b/.github/workflows/predict-conflicts.yml index d2191ed245e4..f9bf9dde931c 100644 --- a/.github/workflows/predict-conflicts.yml +++ b/.github/workflows/predict-conflicts.yml @@ -1,10 +1,11 @@ name: "Check Potential Conflicts" +# A scheduled run is not attached to any PR, so a slow or cancelled run never +# shows up in a PR's checks. Each run rechecks every open PR. on: - pull_request_target: # zizmor: ignore[dangerous-triggers] needs a write token to comment on fork PRs; runs base-branch code only - types: [opened, synchronize, reopened, ready_for_review, converted_to_draft, closed] - pull_request_review: - types: [submitted] + schedule: + - cron: '7,37 * * * *' + workflow_dispatch: permissions: contents: read @@ -21,7 +22,10 @@ permissions: jobs: predict_conflicts: + # Forks also run scheduled workflows. + if: github.repository == 'dashpay/dash' runs-on: ${{ vars.DASH_LIGHT_RUNNER || 'ubuntu-latest' }} + timeout-minutes: 20 concurrency: group: predict-conflicts cancel-in-progress: false @@ -30,15 +34,7 @@ jobs: uses: actions/checkout@v6 with: persist-credentials: false - - name: Validate potential conflicts and update advisory comments - continue-on-error: true + - name: Find conflicting PRs and update advisory comments env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - EVENT_ACTION: ${{ github.event.action }} - PR_NUMBER: ${{ github.event.pull_request.number }} - run: | - if [ "$EVENT_ACTION" = "closed" ]; then - .github/workflows/handle_potential_conflicts.py --pr-number "$PR_NUMBER" --cleanup-closed - else - .github/workflows/handle_potential_conflicts.py --pr-number "$PR_NUMBER" - fi + run: .github/workflows/handle_potential_conflicts.py diff --git a/.github/workflows/test_handle_potential_conflicts.py b/.github/workflows/test_handle_potential_conflicts.py index 2cf5fc04316a..098c06c6bf48 100755 --- a/.github/workflows/test_handle_potential_conflicts.py +++ b/.github/workflows/test_handle_potential_conflicts.py @@ -3,11 +3,15 @@ # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. +import argparse +import base64 +from datetime import datetime, timedelta, timezone import importlib.util +import json from pathlib import Path -import argparse import sys import unittest +from unittest import mock SCRIPT_PATH = Path(__file__).with_name("handle_potential_conflicts.py") @@ -29,372 +33,208 @@ def comment(comment_id, body, user=None): } -class TestPotentialConflictComments(unittest.TestCase): - def test_extract_state_from_managed_comment(self): - body = handle_potential_conflicts.render_comment_body({ - "outbound": [ - {"number": 2, "title": "two", "url": "https://example.test/2", "files": []}, - ], - "inbound": { - "3": {"number": 3, "title": "three", "url": "https://example.test/3", "files": ["src/a.cpp"]}, - }, - }) +def item(number, title="title", files=()): + return { + "number": number, + "title": title, + "url": f"https://github.com/dashpay/dash/pull/{number}", + "files": list(files), + } - state = handle_potential_conflicts.extract_state(body) - self.assertEqual(2, state["outbound"][0]["number"]) - self.assertEqual(3, state["inbound"]["3"]["number"]) +LONG_AGO = "2020-01-01T00:00:00Z" +RECENTLY = (datetime.now(timezone.utc) - timedelta(hours=1)).strftime("%Y-%m-%dT%H:%M:%SZ") - def test_invalid_state_is_treated_as_empty(self): - body = """ -## Potential PR merge conflicts -""" - state = handle_potential_conflicts.extract_state(body) +def pr(number, base="develop", draft=False, updated_at=LONG_AGO): + return { + "number": number, + "title": f"pr {number}", + "html_url": f"https://github.com/dashpay/dash/pull/{number}", + "base": {"ref": base}, + "head": {"label": f"contributor:branch-{number}"}, + "draft": draft, + "updated_at": updated_at, + } - self.assertEqual([], state["outbound"]) - self.assertEqual({}, state["inbound"]) - def test_render_comment_body_has_one_marker_and_both_sections(self): - state = { - "outbound": [ - { - "number": 12, - "title": "fix: target", - "url": "https://github.com/dashpay/dash/pull/12", - "files": ["src/net.cpp"], - } - ], - "inbound": { - "9": { - "number": 9, - "title": "refactor: source", - "url": "https://github.com/dashpay/dash/pull/9", - "files": ["src/net.cpp"], - } - }, - } +def legacy_body(state): + encoded = base64.b64encode(json.dumps(state).encode()).decode() + return f"## Potential PR merge conflicts\n\n{handle_potential_conflicts.COMMENT_START}\n{encoded}\n-->\n" - body = handle_potential_conflicts.render_comment_body(state) - self.assertEqual(1, body.count("dash-potential-conflicts:v1")) - self.assertIn("If this PR merges first", body) - self.assertIn("If these PRs merge first", body) - self.assertIn("[#12: fix: target](https://github.com/dashpay/dash/pull/12)", body) - self.assertIn("[#9: refactor: source](https://github.com/dashpay/dash/pull/9)", body) +def managed(comment_id, conflicts): + body = handle_potential_conflicts.render_comment_body(conflicts) + return handle_potential_conflicts.ManagedComment(comment_id, body, {int(entry["number"]) for entry in conflicts}) - def test_rendered_comment_state_round_trips(self): - state = { - "outbound": [ - { - "number": 12, - "title": f"title with {handle_potential_conflicts.COMMENT_START} --> marker-like text", - "url": "https://github.com/dashpay/dash/pull/12", - "files": ["src/net.cpp"], - } - ], - "inbound": {}, - } - body = handle_potential_conflicts.render_comment_body(state) - extracted = handle_potential_conflicts.extract_state(body) +def numbers(conflicts): + return {number: [entry["number"] for entry in items] for number, items in conflicts.items()} + + +class TestPotentialConflictComments(unittest.TestCase): + def test_rendered_comment_lists_prs_and_round_trips(self): + title = f"title with {handle_potential_conflicts.COMMENT_START} --> marker-like text" + body = handle_potential_conflicts.render_comment_body([item(12, title, ["src/net.cpp"]), item(9, "refactor: source")]) - self.assertEqual(state, extracted) + self.assertEqual({9, 12}, handle_potential_conflicts.extract_listed_prs(body)) self.assertTrue(body.startswith("## Potential PR merge conflicts")) self.assertTrue(body.rstrip("\n").endswith(handle_potential_conflicts.COMMENT_END)) + self.assertLess(body.index("[#9: refactor: source]"), body.index("[#12: title with")) - def test_extract_state_from_legacy_marker_first_comment(self): + def test_extract_listed_prs_from_legacy_comments(self): state = { - "outbound": [ - {"number": 2, "title": "two", "url": "https://example.test/2", "files": []}, - ], - "inbound": {}, + "outbound": [item(2), {"title": "missing number"}, "not-a-dict"], + "inbound": {"3": item(3), "bad": {"title": "missing number"}}, } - body = handle_potential_conflicts.render_comment_body(state) + body = legacy_body(state) marker_index = body.rfind(handle_potential_conflicts.COMMENT_START) - legacy_body = body[marker_index:] + body[:marker_index] + marker_first_body = body[marker_index:] + body[:marker_index] - self.assertTrue(legacy_body.startswith(handle_potential_conflicts.COMMENT_START)) - self.assertEqual(state, handle_potential_conflicts.extract_state(legacy_body)) + self.assertEqual({2, 3}, handle_potential_conflicts.extract_listed_prs(body)) + self.assertEqual({2, 3}, handle_potential_conflicts.extract_listed_prs(marker_first_body)) + + def test_invalid_state_is_treated_as_empty(self): + body = """ +## Potential PR merge conflicts +""" + + self.assertEqual(set(), handle_potential_conflicts.extract_listed_prs(body)) def test_rendered_comment_escapes_markdown_content(self): - body = handle_potential_conflicts.render_comment_body({ - "outbound": [ - { - "number": 12, - "title": "fix [link] title", - "url": "https://github.com/dashpay/dash/pull/12", - "files": ["src/`odd`.cpp"], - }, - ], - "inbound": {}, - }) + body = handle_potential_conflicts.render_comment_body([item(12, "fix [link] title", ["src/`odd`.cpp"])]) self.assertIn(r"[#12: fix \[link\] title](https://github.com/dashpay/dash/pull/12)", body) self.assertIn("src/`odd`.cpp", body) - def test_normalize_state_filters_malformed_entries(self): - state = handle_potential_conflicts.normalize_state({ - "outbound": [ - {"number": "12", "title": "valid", "url": "https://example.test/12", "files": ["src/a.cpp"]}, - {"title": "missing number"}, - "not-a-dict", - ], - "inbound": { - "bad": {"title": "missing number"}, - "13": {"number": "13", "title": "valid inbound", "url": "https://example.test/13", "files": []}, - }, - }) - - self.assertEqual([12], [item["number"] for item in state["outbound"]]) - self.assertEqual(["13"], list(state["inbound"].keys())) - - def test_state_is_empty_only_when_both_directions_are_empty(self): - self.assertTrue(handle_potential_conflicts.state_is_empty({"outbound": [], "inbound": {}})) - self.assertFalse(handle_potential_conflicts.state_is_empty({"outbound": [{"number": 1}], "inbound": {}})) - self.assertFalse(handle_potential_conflicts.state_is_empty({"outbound": [], "inbound": {"2": {"number": 2}}})) - def test_user_forged_managed_comment_is_ignored(self): - body = handle_potential_conflicts.render_comment_body({ - "outbound": [{"number": 12, "title": "forged", "url": "https://example.test/12", "files": []}], - "inbound": {}, - }) - comments = [comment(123, body, USER)] - original_list_issue_comments = handle_potential_conflicts.list_issue_comments - handle_potential_conflicts.list_issue_comments = lambda pr_num: comments - try: - managed_comments = handle_potential_conflicts.find_managed_comments(1) - finally: - handle_potential_conflicts.list_issue_comments = original_list_issue_comments - - self.assertEqual([], managed_comments) - - def test_update_comments_replaces_reciprocal_entries(self): - our_pr_json = { - "number": 10, - "title": "fix: source", - "html_url": "https://github.com/dashpay/dash/pull/10", - } - validated_conflicts = [ - { - "number": 12, - "title": "fix: new target", - "url": "https://github.com/dashpay/dash/pull/12", - "files": ["src/net.cpp"], - } - ] + body = handle_potential_conflicts.render_comment_body([item(12, "forged")]) + with mock.patch.object(handle_potential_conflicts, "list_issue_comments", return_value=[comment(123, body, USER)]): + self.assertEqual([], handle_potential_conflicts.find_managed_comments(1)) + + def save(self, comments, conflicts): + with mock.patch.object(handle_potential_conflicts, "github_patch") as patch, \ + mock.patch.object(handle_potential_conflicts, "github_delete") as delete, \ + mock.patch.object(handle_potential_conflicts, "github_post") as post: + handle_potential_conflicts.save_managed_comment(10, comments, conflicts) + return ([call.args[0] for call in post.call_args_list], + [call.args[0] for call in patch.call_args_list], + [call.args[0] for call in delete.call_args_list]) + + def test_save_managed_comment_writes_only_what_changed(self): + self.assertEqual((["issues/10/comments"], [], []), self.save([], [item(13)])) + self.assertEqual(([], ["issues/comments/3"], ["issues/comments/1"]), + self.save([managed(1, [item(11)]), managed(3, [item(12)])], [item(13)])) + self.assertEqual(([], [], []), self.save([managed(3, [item(13)])], [item(13)])) + self.assertEqual(([], [], ["issues/comments/1", "issues/comments/3"]), + self.save([managed(1, [item(11)]), managed(3, [item(12)])], [])) + + def find_conflicts(self, prs, files, mergeable_state, can_merge): + def github_get(path): + number = int(path.removeprefix("pulls/")) + if mergeable_state[number] is None: + raise OSError("boom") + return {"mergeable_state": mergeable_state[number]} + + with mock.patch.object(handle_potential_conflicts, "get_pr_files", side_effect=files.get), \ + mock.patch.object(handle_potential_conflicts, "github_get", side_effect=github_get), \ + mock.patch.object(handle_potential_conflicts, "branches_can_merge", + side_effect=lambda a, b: can_merge[(int(a.split("-")[-1]), int(b.split("-")[-1]))]) as check: + conflicts, unchecked = handle_potential_conflicts.find_conflicts(prs) + + checked = [tuple(int(label.split("-")[-1]) for label in call.args) for call in check.call_args_list] + return checked, numbers(conflicts), {number: sorted(items) for number, items in unchecked.items() if items} + + def test_find_conflicts_checks_each_overlapping_pair_once(self): + # 4 shares a file with 1 but targets another base; 5 shares nothing with 1-3. + prs = [pr(1), pr(2), pr(3), pr(4, base="v23.x"), pr(5)] + files = {1: {"a"}, 2: {"a", "b"}, 3: {"b"}, 4: {"a"}, 5: {"c"}} + mergeable_state = {1: "clean", 2: "blocked", 3: "unstable", 5: "clean"} + checked, conflicts, unchecked = self.find_conflicts(prs, files, mergeable_state, {(1, 2): False, (2, 3): True}) + + self.assertEqual([(1, 2), (2, 3)], checked) + self.assertEqual({1: [2], 2: [1], 3: [], 4: [], 5: []}, conflicts) + self.assertEqual({}, unchecked) + + def test_find_conflicts_leaves_pairs_it_cannot_check_unchecked(self): + # The merge check fails for 1 and 2; 3 needs a rebase, 4 could not be fetched, + # GitHub has not yet checked 5 against its base, and 8's files could not be listed, + # so their pairs are never checked. + prs = [pr(number) for number in range(1, 9)] + [pr(9, base="v23.x")] + files = {1: {"a"}, 2: {"a"}, 3: {"b"}, 4: {"b"}, 5: {"c"}, 6: {"c"}, 7: {"b"}, 8: None, 9: {"a"}} + mergeable_state = {1: "clean", 2: "clean", 3: "dirty", 4: None, 5: "unknown", 6: "clean", 7: "clean"} + checked, conflicts, unchecked = self.find_conflicts(prs, files, mergeable_state, {(1, 2): None}) + + self.assertEqual([(1, 2)], checked) + self.assertEqual({number: [] for number in range(1, 10)}, conflicts) + self.assertEqual({1: [2, 8], 2: [1, 8], 3: [4, 7, 8], 4: [3, 7, 8], 5: [6, 8], 6: [5, 8], 7: [3, 4, 8], + 8: [1, 2, 3, 4, 5, 6, 7]}, unchecked) + + with mock.patch.object(handle_potential_conflicts, "github_get_paginated", side_effect=OSError("boom")): + self.assertIsNone(handle_potential_conflicts.get_pr_files(8)) + + def run_main(self, open_prs, conflicts, unchecked, find_managed_comments, closed_prs=(), dry_run=False): + saved = [] + exit_code = None + with mock.patch.object(handle_potential_conflicts, "parse_args", return_value=argparse.Namespace(dry_run=dry_run)), \ + mock.patch.object(handle_potential_conflicts, "github_get_paginated", return_value=open_prs), \ + mock.patch.object(handle_potential_conflicts, "github_get", return_value=list(closed_prs)), \ + mock.patch.object(handle_potential_conflicts, "find_conflicts", return_value=(conflicts, unchecked)) as find, \ + mock.patch.object(handle_potential_conflicts, "find_managed_comments", side_effect=find_managed_comments), \ + mock.patch.object(handle_potential_conflicts, "save_managed_comment", + side_effect=lambda number, _comments, items: saved.append((number, [entry["number"] for entry in items]))), \ + mock.patch("sys.stdout"): + try: + handle_potential_conflicts.main() + except SystemExit as e: + exit_code = e.code + return [entry["number"] for entry in find.call_args.args[0]], saved, exit_code + + def test_main_rewrites_comments_and_clears_prs_no_longer_checked(self): + # 7 became a draft and 8 was closed since the comments were written. 1 and 3 could + # not be checked against 4 this run; only 1's comment already listed 4. 5 and 6 listed + # only each other and both closed recently, and 9 was made a draft recently, so nothing + # lists them any more; 10 was closed long ago. comments = { - 10: handle_potential_conflicts.ManagedComment( - comment_id=1, - body="", - state={ - "outbound": [ - { - "number": 11, - "title": "fix: stale target", - "url": "https://github.com/dashpay/dash/pull/11", - "files": ["src/old.cpp"], - } - ], - "inbound": { - "8": { - "number": 8, - "title": "fix: incoming", - "url": "https://github.com/dashpay/dash/pull/8", - "files": ["src/in.cpp"], - } - }, - }, - ), - 11: handle_potential_conflicts.ManagedComment(comment_id=2, body="", state={"outbound": [], "inbound": {"10": {"number": 10}}}), - 12: handle_potential_conflicts.ManagedComment(comment_id=3, body="", state={"outbound": [], "inbound": {}}), + 1: [managed(11, [item(2), item(4), item(7)])], + 2: [managed(12, [item(1), item(8)])], + 7: [managed(17, [item(1)])], + 8: [managed(18, [item(2)])], } - saved = {} - original_get_managed_comment = handle_potential_conflicts.get_managed_comment - original_save_managed_comment = handle_potential_conflicts.save_managed_comment - handle_potential_conflicts.get_managed_comment = lambda pr_num: comments[pr_num] - handle_potential_conflicts.save_managed_comment = lambda pr_num, state: saved.setdefault(pr_num, state) - try: - handle_potential_conflicts.update_comments(our_pr_json, validated_conflicts) - finally: - handle_potential_conflicts.get_managed_comment = original_get_managed_comment - handle_potential_conflicts.save_managed_comment = original_save_managed_comment - - self.assertEqual([12], [item["number"] for item in saved[10]["outbound"]]) - self.assertIn("8", saved[10]["inbound"]) - self.assertNotIn("10", saved[11]["inbound"]) - self.assertEqual(10, saved[12]["inbound"]["10"]["number"]) - - def test_cleanup_closed_pr_removes_bidirectional_state(self): - comments = { - 10: handle_potential_conflicts.ManagedComment( - comment_id=10, - body="managed", - state={ - "outbound": [ - { - "number": 12, - "title": "fix: target", - "url": "https://github.com/dashpay/dash/pull/12", - "files": ["src/net.cpp"], - } - ], - "inbound": { - "8": { - "number": 8, - "title": "fix: source", - "url": "https://github.com/dashpay/dash/pull/8", - "files": ["src/net.cpp"], - } - }, - }, - ), - 8: handle_potential_conflicts.ManagedComment( - comment_id=8, - body="managed", - state={ - "outbound": [ - {"number": 10, "title": "fix: closed target", "url": "https://github.com/dashpay/dash/pull/10", "files": []}, - {"number": 11, "title": "fix: other target", "url": "https://github.com/dashpay/dash/pull/11", "files": []}, - ], - "inbound": {}, - }, - ), - 12: handle_potential_conflicts.ManagedComment( - comment_id=12, - body="managed", - state={"outbound": [], "inbound": {"10": {"number": 10}}}, - ), - } - saved = {} - deleted = [] - original_get_managed_comment = handle_potential_conflicts.get_managed_comment - original_find_managed_comments = handle_potential_conflicts.find_managed_comments - original_save_managed_comment = handle_potential_conflicts.save_managed_comment - original_github_delete = handle_potential_conflicts.github_delete - handle_potential_conflicts.get_managed_comment = lambda pr_num: comments[pr_num] - handle_potential_conflicts.find_managed_comments = lambda pr_num: [comments[pr_num]] - handle_potential_conflicts.save_managed_comment = lambda pr_num, state: saved.setdefault(pr_num, state) - handle_potential_conflicts.github_delete = lambda path: deleted.append(path) - try: - handle_potential_conflicts.cleanup_closed_pr_comments(10) - finally: - handle_potential_conflicts.get_managed_comment = original_get_managed_comment - handle_potential_conflicts.find_managed_comments = original_find_managed_comments - handle_potential_conflicts.save_managed_comment = original_save_managed_comment - handle_potential_conflicts.github_delete = original_github_delete - - self.assertEqual([11], [item["number"] for item in saved[8]["outbound"]]) - self.assertNotIn("10", saved[12]["inbound"]) - self.assertEqual(["issues/comments/10"], deleted) - - def test_comment_fetch_failure_prevents_comment_creation(self): - posted = [] - original_list_issue_comments = handle_potential_conflicts.list_issue_comments - original_github_post = handle_potential_conflicts.github_post - handle_potential_conflicts.list_issue_comments = lambda pr_num: (_ for _ in ()).throw(OSError("boom")) - handle_potential_conflicts.github_post = lambda path, payload: posted.append((path, payload)) - try: - with self.assertRaises(OSError): - handle_potential_conflicts.save_managed_comment( - 10, - { - "outbound": [ - { - "number": 12, - "title": "fix: target", - "url": "https://github.com/dashpay/dash/pull/12", - "files": [], - } - ], - "inbound": {}, - }, - ) - finally: - handle_potential_conflicts.list_issue_comments = original_list_issue_comments - handle_potential_conflicts.github_post = original_github_post - - self.assertEqual([], posted) - - def test_main_cleans_current_draft_pr(self): - cleaned = [] - original_parse_args = handle_potential_conflicts.parse_args - original_get_pr_json = handle_potential_conflicts.get_pr_json - original_cleanup_closed_pr_comments = handle_potential_conflicts.cleanup_closed_pr_comments - handle_potential_conflicts.parse_args = lambda: argparse.Namespace( - pr_number=10, - dry_run=False, - cleanup_closed=False, + checked, saved, exit_code = self.run_main( + [pr(1), pr(2), pr(3), pr(4), pr(7, draft=True), pr(9, draft=True, updated_at=RECENTLY)], + {1: [item(2)], 2: [item(1)], 3: [], 4: []}, + {1: {4: item(4)}, 2: {}, 3: {4: item(4)}, 4: {1: item(1), 3: item(3)}}, + lambda number: comments.get(number, []), + closed_prs=[pr(5, updated_at=RECENTLY), pr(6, updated_at=RECENTLY), pr(10)], ) - handle_potential_conflicts.get_pr_json = lambda pr_num: { - "number": pr_num, - "draft": True, - "head": {"label": "contributor:branch"}, - } - handle_potential_conflicts.cleanup_closed_pr_comments = lambda pr_num: cleaned.append(pr_num) - try: - handle_potential_conflicts.main() - finally: - handle_potential_conflicts.parse_args = original_parse_args - handle_potential_conflicts.get_pr_json = original_get_pr_json - handle_potential_conflicts.cleanup_closed_pr_comments = original_cleanup_closed_pr_comments - - self.assertEqual([10], cleaned) - - def test_save_managed_comment_deletes_duplicate_managed_comments(self): - comments = [ - comment( - 1, - handle_potential_conflicts.render_comment_body({ - "outbound": [{"number": 11, "title": "old", "url": "https://example.test/11", "files": []}], - "inbound": {}, - }), - ), - comment( - 3, - handle_potential_conflicts.render_comment_body({ - "outbound": [{"number": 12, "title": "old", "url": "https://example.test/12", "files": []}], - "inbound": {}, - }), - ), - ] - patched = [] - deleted = [] - posted = [] - original_list_issue_comments = handle_potential_conflicts.list_issue_comments - original_github_patch = handle_potential_conflicts.github_patch - original_github_delete = handle_potential_conflicts.github_delete - original_github_post = handle_potential_conflicts.github_post - handle_potential_conflicts.list_issue_comments = lambda pr_num: comments - handle_potential_conflicts.github_patch = lambda path, payload: patched.append((path, payload)) - handle_potential_conflicts.github_delete = lambda path: deleted.append(path) - handle_potential_conflicts.github_post = lambda path, payload: posted.append((path, payload)) - try: - handle_potential_conflicts.save_managed_comment( - 10, - { - "outbound": [ - { - "number": 13, - "title": "new", - "url": "https://example.test/13", - "files": [], - } - ], - "inbound": {}, - }, - ) - finally: - handle_potential_conflicts.list_issue_comments = original_list_issue_comments - handle_potential_conflicts.github_patch = original_github_patch - handle_potential_conflicts.github_delete = original_github_delete - handle_potential_conflicts.github_post = original_github_post - - self.assertEqual(["issues/comments/3"], [path for path, _payload in patched]) - self.assertEqual(["issues/comments/1"], deleted) - self.assertEqual([], posted) + + self.assertEqual([1, 2, 3, 4], checked) + self.assertEqual([(1, [2, 4]), (2, [1]), (3, []), (4, []), (5, []), (6, []), (7, []), (8, []), (9, [])], saved) + self.assertIsNone(exit_code) + + def test_main_does_not_write_a_comment_it_could_not_read(self): + def find_managed_comments(number): + if number in (1, 5): + raise OSError("boom") + return [] + + _checked, saved, exit_code = self.run_main( + [pr(1), pr(2)], {1: [item(2)], 2: [item(1)]}, {1: {}, 2: {}}, find_managed_comments, + closed_prs=[pr(5, updated_at=RECENTLY)]) + + self.assertEqual([(2, [1])], saved) + self.assertIn("[1, 5]", exit_code) + + def test_dry_run_writes_nothing(self): + _checked, saved, exit_code = self.run_main( + [pr(1), pr(2)], {1: [item(2)], 2: [item(1)]}, {1: {}, 2: {}}, lambda number: [managed(number, [item(8)])], + closed_prs=[pr(5, updated_at=RECENTLY)], dry_run=True) + + self.assertEqual([], saved) + self.assertIsNone(exit_code) def test_format_files_limits_long_file_lists(self): files = [f"src/file_{i}.cpp" for i in range(handle_potential_conflicts.MAX_FILE_DETAILS + 2)]