diff --git a/.github/workflows/1_ear_bot_pr.yml b/.github/workflows/1_ear_bot_pr.yml index 50aef78d..3a07701a 100644 --- a/.github/workflows/1_ear_bot_pr.yml +++ b/.github/workflows/1_ear_bot_pr.yml @@ -10,6 +10,8 @@ concurrency: group: ear-bot-pr-${{ github.event.pull_request.number }} cancel-in-progress: false +permissions: {} + jobs: add-label-and-find-supervisor: runs-on: ubuntu-latest diff --git a/.github/workflows/2+4_ear_bot_comment.yml b/.github/workflows/2+4_ear_bot_comment.yml index d9be7a12..432a03a1 100644 --- a/.github/workflows/2+4_ear_bot_comment.yml +++ b/.github/workflows/2+4_ear_bot_comment.yml @@ -8,6 +8,8 @@ concurrency: group: ear-bot-pr-${{ github.event.issue.number }} cancel-in-progress: false +permissions: {} + jobs: new-comment: if: github.actor != 'erga-ear-bot[bot]' && github.event.issue.pull_request && (contains(github.event.issue.labels.*.name, 'ERGA-BGE') || contains(github.event.issue.labels.*.name, 'ERGA-Pilot') || contains(github.event.issue.labels.*.name, 'ERGA-Community')) diff --git a/.github/workflows/3_ear_bot_reviewer.yml b/.github/workflows/3_ear_bot_reviewer.yml index afdc351d..0aaebc3b 100644 --- a/.github/workflows/3_ear_bot_reviewer.yml +++ b/.github/workflows/3_ear_bot_reviewer.yml @@ -10,6 +10,8 @@ concurrency: group: ear-bot-scheduled cancel-in-progress: false +permissions: {} + jobs: find-reviewer: runs-on: ubuntu-latest diff --git a/.github/workflows/5_ear_bot_approved.yml b/.github/workflows/5_ear_bot_approved.yml index 78be6b14..f0e52acb 100644 --- a/.github/workflows/5_ear_bot_approved.yml +++ b/.github/workflows/5_ear_bot_approved.yml @@ -8,6 +8,12 @@ concurrency: group: ear-bot-pr-${{ github.event.pull_request.number }} cancel-in-progress: false +# Unlike the other workflows, this one's checkout has no `token:` input, so it +# relies on the ambient GITHUB_TOKEN. Granting contents: read explicitly rather +# than depending on the public-repo default, which is not clearly documented. +permissions: + contents: read + jobs: approved-changes: if: github.event.review.state == 'approved' && (contains(github.event.pull_request.labels.*.name, 'ERGA-BGE') || contains(github.event.pull_request.labels.*.name, 'ERGA-Pilot') || contains(github.event.pull_request.labels.*.name, 'ERGA-Community')) diff --git a/.github/workflows/5_ear_bot_approved_comment.yml b/.github/workflows/5_ear_bot_approved_comment.yml index baa9b50f..c911d498 100644 --- a/.github/workflows/5_ear_bot_approved_comment.yml +++ b/.github/workflows/5_ear_bot_approved_comment.yml @@ -10,6 +10,9 @@ concurrency: group: ear-bot-approved-${{ github.event.workflow_run.id }} cancel-in-progress: false +permissions: + actions: read + jobs: approved-changes: if: ${{ github.event.workflow_run.conclusion == 'success' }} diff --git a/.github/workflows/6_ear_bot_merge.yml b/.github/workflows/6_ear_bot_merge.yml index c8d5d3f2..267667f4 100644 --- a/.github/workflows/6_ear_bot_merge.yml +++ b/.github/workflows/6_ear_bot_merge.yml @@ -8,6 +8,8 @@ concurrency: group: ear-bot-pr-${{ github.event.pull_request.number }} cancel-in-progress: false +permissions: {} + jobs: closed-pr: if: contains(github.event.pull_request.labels.*.name, 'ERGA-BGE') || contains(github.event.pull_request.labels.*.name, 'ERGA-Pilot') || contains(github.event.pull_request.labels.*.name, 'ERGA-Community') || contains(github.event.pull_request.labels.*.name, 'EAR-UPDATE') diff --git a/.gitignore b/.gitignore index e43b0f98..63c152ce 100644 --- a/.gitignore +++ b/.gitignore @@ -1 +1,8 @@ .DS_Store +__pycache__/ +*.py[cod] +.mypy_cache/ +.ruff_cache/ +.pytest_cache/ +.venv/ +venv/ diff --git a/ear_bot/__init__.py b/ear_bot/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/ear_bot/ear_bot_reviewer.py b/ear_bot/ear_bot_reviewer.py index b121e92b..ad6200dc 100644 --- a/ear_bot/ear_bot_reviewer.py +++ b/ear_bot/ear_bot_reviewer.py @@ -40,10 +40,12 @@ from datetime import datetime, timedelta import pytz -from github import Auth, Github, UnknownObjectException +from github import Auth, Github root_folder = os.path.abspath(os.path.join(os.path.dirname(__file__), "..")) sys.path.append(root_folder) +from ear_bot import roster as roster_module # noqa: E402 +from ear_bot.roster import Roster, replace # noqa: E402 from rev import get_EAR_reviewer # noqa: E402 cet = pytz.timezone("CET") @@ -53,69 +55,59 @@ def _get_local_path(filename): return os.path.join(root_folder, filename) -def commit(repo, path, message, content): - try: - contents = repo.get_contents(path) - if not isinstance(contents, list): - repo.update_file(contents.path, message, content, contents.sha) - print(f"Updated {path} file.") - else: - print(f"{path} file could not be updated.") - except UnknownObjectException: - try: - repo.create_file(path, message, content) - print(f"Created {path} file.") - except Exception as e: - print(f"Error creating {path} file.\n\n{content}\n\n\n{e}") - except Exception as e: - print(f"Error updating {path} file.\n{e}") +class EARBotReviewer: + """Main bot class — one instance per workflow run. + Reads all required state from environment variables (set by the calling + workflow) and from the GitHub API via PyGithub. Each public method + corresponds to one CLI flag and one workflow stage. + """ -class EAR_get_reviewer: - """Thin wrapper around the CSV data files and get_EAR_reviewer helpers. + EARPDF_TO_YAML_SCRIPT = "EARpdf_to_yaml.py" - Fetches ``rev/reviewers_list.csv`` from the GitHub repo on construction - and exposes methods to select supervisors/reviewers and to commit updates - back to the repo. + # "no problem", "no worries" and friends are agreement, not refusal. + # Matching a bare \bno\b read "Sure, no problem" as a decline and handed + # the assembly to somebody else. + YES_NO = { + "yes": r"\byes\b", + "no": r"\bno\b(?!\s+(?:problem|worries|issue|trouble|objection))", + } - To point the bot at different CSV paths, change the class-level constants - REVIEWERS_CSV and EAR_REVIEWS_CSV. - """ + def __init__(self) -> None: + token = os.getenv("GITHUB_APP_TOKEN") + if not token: + raise Exception("GITHUB_APP_TOKEN environment variable is not set") + g = Github(auth=Auth.Token(token)) + self.repo = g.get_repo(str(os.getenv("GITHUB_REPOSITORY"))) + self.roster = Roster(self.repo) + self.pr_number = os.getenv("PR_NUMBER") + self.comment_text = os.getenv("COMMENT_TEXT") + self.comment_author = os.getenv("COMMENT_AUTHOR") + self.reviewer = os.getenv("REVIEWER") + self.valid_projects = ["ERGA-BGE", "ERGA-Pilot", "ERGA-Community"] - REVIEWERS_CSV = "rev/reviewers_list.csv" - EAR_REVIEWS_CSV = "rev/EAR_reviews.csv" GET_EAR_REVIEWER_SCRIPT = "rev/get_EAR_reviewer.py" - def __init__(self, repo) -> None: - self.repo = repo - csv_data = self._fetch_csv_from_repo(self.REVIEWERS_CSV) - self.data = get_EAR_reviewer.parse_csv(csv_data) - - def _fetch_csv_from_repo(self, csv_path): - contents = self.repo.get_contents(csv_path) - if isinstance(contents, list): - raise Exception(f"Expected a file, got a directory: {csv_path}") - csv_data = contents.decoded_content.decode("utf-8") - if not csv_data: - raise Exception(f"The CSV file is empty: {csv_path}") - return csv_data - - def get_supervisor(self, user, calling_institution): + def select_supervisor(self, user, calling_institution): try: - selected_supervisor = get_EAR_reviewer.select_random_supervisor( - self.data, user, calling_institution + selected = get_EAR_reviewer.select_random_supervisor( + self.roster.data, user, calling_institution ) - if selected_supervisor is None: + if selected is None: raise Exception("No supervisor selected") - return selected_supervisor.get("Github ID") + return selected.get("Github ID") except Exception as e: raise Exception(f"No eligible supervisors found.\n{e}") - def get_reviewer(self, institution, project): + def select_reviewer(self, institution, project): try: _, top_candidate, _ = get_EAR_reviewer.select_best_reviewer( - self.data, institution, project + self.roster.data, institution, project ) + if not top_candidate: + raise Exception( + f"No active reviewer outside {institution!r} is available." + ) reviewer_print = subprocess.run( f"python {_get_local_path(self.GET_EAR_REVIEWER_SCRIPT)} -i '{institution}' -t '{project}'", shell=True, @@ -126,103 +118,6 @@ def get_reviewer(self, institution, project): except Exception as e: raise Exception(f"No eligible candidates found.\n{e}") - def add_pr( - self, - pr_url, - species, - tag, - requester_name, - requester_affiliation, - reviewer_name, - reviewer_affiliation, - supervisor_name, - supervisor_affiliation, - interaction_count, - open_date, - approval_date, - other_participants, - notes, - ): - ear_reviews_csv_data = self._fetch_csv_from_repo(self.EAR_REVIEWS_CSV) - - csv_row = ( - f"{pr_url},{species},{tag},{requester_name}," - f"{requester_affiliation},{reviewer_name},{reviewer_affiliation}," - f"{supervisor_name},{supervisor_affiliation},{interaction_count}," - f'{open_date},{approval_date},"{other_participants}",{notes}\n' - ) - ear_reviews_csv_data += csv_row - commit( - self.repo, self.EAR_REVIEWS_CSV, "Add new EAR review", ear_reviews_csv_data - ) - print(f"Added {reviewer_name} to the EAR reviews CSV file.") - - def update_reviewers_list( - self, - reviewers, - busy, - institution="", - submitted_at="", - fined_reviewers=set(), - ): - if not reviewers: - print("No reviewers to update.") - return - for reviewer_data in self.data: - reviewer_data_id = reviewer_data.get("Github ID", "").lower() - reviewer_data_score = int(reviewer_data.get("Calling Score", 1000)) - reviewer_data_total = int(reviewer_data.get("Total Reviews", 0)) - reviewer_data_institution = reviewer_data.get("Institution", "").lower() - working_prs = int(reviewer_data.get("Working PRs", 0)) - - if reviewer_data_id in reviewers: - if busy: - working_prs += 1 - else: - working_prs = max(0, working_prs - 1) - reviewer_data["Working PRs"] = str(working_prs) - - if submitted_at: - reviewer_data_score -= 1 - reviewer_data["Calling Score"] = str(reviewer_data_score) - reviewer_data["Total Reviews"] = str(reviewer_data_total + 1) - reviewer_data["Last Review"] = submitted_at - elif reviewer_data_id in fined_reviewers: - reviewer_data_score += 1 - reviewer_data["Calling Score"] = str(reviewer_data_score) - if reviewer_data_institution == institution.lower(): - reviewer_data["Calling Score"] = str(reviewer_data_score + 1) - - csv_str = ",".join(self.data[0].keys()) + "\n" - for row in self.data: - csv_str += ",".join(row.values()) + "\n" - commit(self.repo, self.REVIEWERS_CSV, "Update reviewers list", csv_str) - print(f"Updated the reviewers list for {', '.join(reviewers)}.\n{csv_str}") - - -class EARBotReviewer: - """Main bot class — one instance per workflow run. - - Reads all required state from environment variables (set by the calling - workflow) and from the GitHub API via PyGithub. Each public method - corresponds to one CLI flag and one workflow stage. - """ - - EARPDF_TO_YAML_SCRIPT = "EARpdf_to_yaml.py" - - def __init__(self) -> None: - token = os.getenv("GITHUB_APP_TOKEN") - if not token: - raise Exception("GITHUB_APP_TOKEN environment variable is not set") - g = Github(auth=Auth.Token(token)) - self.repo = g.get_repo(str(os.getenv("GITHUB_REPOSITORY"))) - self.EAR_reviewer = EAR_get_reviewer(self.repo) - self.pr_number = os.getenv("PR_NUMBER") - self.comment_text = os.getenv("COMMENT_TEXT") - self.comment_author = os.getenv("COMMENT_AUTHOR") - self.reviewer = os.getenv("REVIEWER") - self.valid_projects = ["ERGA-BGE", "ERGA-Pilot", "ERGA-Community"] - def find_supervisor(self): """Validate the PR and request supervisor confirmation (--supervisor flag). @@ -304,7 +199,7 @@ def find_supervisor(self): "do you agree to [supervise]" in comment.body for comment in pr.get_issue_comments().reversed ): - supervisor = self.EAR_reviewer.get_supervisor( + supervisor = self.select_supervisor( researcher, calling_institution ) pr.create_issue_comment( @@ -341,10 +236,14 @@ def find_reviewer(self, prs=[], reject=False): reject: If True, treats the current reviewer as having declined (used when comment() receives a "No" reply). - Skips PRs that already have a pending review request, an accepted - review, no project label, or no assigned supervisor. Calls - _check_pr_activity() on every PR regardless of skip conditions to - keep DELAYED/STALLED labels up to date. + Two separate gates. A PR the bot does not manage at all -- no project + label, not EAR-UPDATE, not ERROR! -- is skipped outright. EAR-UPDATE + and ERROR! PRs then get _check_pr_activity() so their DELAYED/STALLED + labels and weekly ping keep working, but go no further, because they + have no project label and so no reviewer to assign. Project PRs get + the activity check too, and are then skipped for reviewer selection if + they have a pending review request, a review already in progress, or + no assigned supervisor. Customisation: the 100-working-hour deadline comes from _deadline(); change the ``timedelta(hours=100)`` there to adjust the timeout window. @@ -356,13 +255,20 @@ def find_reviewer(self, prs=[], reject=False): current_date = datetime.now(tz=cet) for pr in prs: + labels = {label.name for label in pr.get_labels()} + # Inactivity tracking covers every PR the bot manages, which + # includes EAR-UPDATE and ERROR! PRs that never get a project + # label. It is deliberately narrower than it once was: this used + # to run on every open PR in the repo, which is why the bot spent + # months posting weekly pings at Dependabot. + if not labels & (set(self.valid_projects) | {"EAR-UPDATE", "ERROR!"}): + continue self._check_pr_activity(pr, current_date) + if not labels & set(self.valid_projects): + continue if ( pr.get_review_requests()[0].totalCount > 0 - or not any( - label.name in self.valid_projects for label in pr.get_labels() - ) - or pr.get_reviews().totalCount > 0 + or self._review_in_progress(pr) or not pr.assignees ): continue @@ -388,7 +294,7 @@ def find_reviewer(self, prs=[], reject=False): if deadline_passed or reject or not old_reviewers_list: new_reviewer, get_EAR_reviewer_print = ( - self.EAR_reviewer.get_reviewer(institution, project) + self.select_reviewer(institution, project) ) if new_reviewer is None: raise Exception("No reviewer found") @@ -398,7 +304,7 @@ def find_reviewer(self, prs=[], reject=False): "Please reply to this message only with **Yes** or **No** by" f" {self._deadline(current_date).strftime('%d-%b-%Y at %H:%M CET')}" ) - self.EAR_reviewer.update_reviewers_list( + self.roster.apply( reviewers=[new_reviewer.lower()], busy=True ) except Exception as e: @@ -440,29 +346,34 @@ def comment(self): print(f"Missing required environment variables.\n{e}") sys.exit(1) + # GitHub logins are case-insensitive, and the roster spelling does not + # always match the login casing, so compare everything lower-cased. supervisors = [ - reviewer["Github ID"] - for reviewer in self.EAR_reviewer.data - if reviewer["Supervisor"] == "Y" and reviewer["Github ID"] != pr.user.login + reviewer["Github ID"].lower() + for reviewer in self.roster.data + if reviewer["Supervisor"] == "Y" + and reviewer["Github ID"].lower() != pr.user.login.lower() ] if ( comment_author in supervisors - and "@erga-ear-bot clear" in comment_text + and self._says(comment_text, r"@erga-ear-bot\s+clear") and pr.state == "closed" ): comment_reviewer = self._search_comment_user(pr, "do you agree to review") - self.EAR_reviewer.update_reviewers_list( - reviewers=set(comment_reviewer), busy=False + self.roster.apply( + reviewers=set(comment_reviewer), busy=False, strict=False ) for label in pr.get_labels(): pr.remove_from_labels(label) + print("Cleared the active tasks for this PR.") + sys.exit() if not pr.assignees: if comment_author not in supervisors: print("The comment author is not one of the supervisors.") sys.exit() - if "ok" in comment_text: + if self._says(comment_text, r"\bok(ay)?\b"): pr.add_to_assignees(comment_author) if not self.pr_number: raise Exception("PR_NUMBER is not set") @@ -478,7 +389,7 @@ def comment(self): if pr.get_review_requests()[0].totalCount > 0: print("The PR is already assigned to a reviewer.") sys.exit() - if pr.get_reviews().totalCount > 0: + if self._review_in_progress(pr): print("The PR already has a review.") sys.exit() if comment_author in map( @@ -494,21 +405,19 @@ def comment(self): print("The reviewer is not the one who was asked to review the PR.") sys.exit() - first_line = "" - for line in comment_text.split("\n"): - stripped = line.strip() - if stripped and not stripped.startswith(">"): - first_line = stripped - break + answer = self._decision( + comment_text, self.YES_NO + ) - if bool(re.search(r"\byes\b", first_line)): + if answer == "yes": time_wasted_reviewers = set( self._search_comment_user(pr, "Time is out!") ) - self.EAR_reviewer.update_reviewers_list( + self.roster.apply( reviewers=set(comment_reviewer) - set([comment_author]), busy=False, fined_reviewers=time_wasted_reviewers, + strict=False, ) pr.create_review_request([comment_author]) pr.create_issue_comment( @@ -520,7 +429,7 @@ def comment(self): " be able to click on the link to the contact map file!)\n" "Contact the PR assignee for any issues." ) - elif bool(re.search(r"\bno\b", first_line)): + elif answer == "no": self.find_reviewer([pr], reject=True) else: current_date = datetime.now(tz=cet) @@ -553,13 +462,32 @@ def approve_reviewer(self): except Exception as e: print(f"Missing required environment variables.\n{e}") sys.exit(1) + if not pr.assignee: + print("The PR has no assigned supervisor yet.") + sys.exit() supervisor = pr.assignee.login researcher = pr.user.login - comment_reviewer = pr.get_reviews() - if comment_reviewer.totalCount == 0 or ( - comment_reviewer.totalCount > 0 - and comment_reviewer[0].user.login.lower() != reviewer - ): + # Check the approver against the reviewer the bot actually appointed. + # REVIEWER comes from github.event.review.user.login and the workflow + # already gates on state == 'approved', so looking for that review in + # pr.get_reviews() would always succeed and constrain nothing. Any + # passer-by can approve a PR on a public repo. + # Anyone currently on the hook counts, plus anyone a review was ever + # requested from. Checking only the bot's most recent ask rejected a + # reviewer the supervisor assigned by hand after the bot's candidate + # declined, and that rejection cascaded: with no thank-you posted, + # closed_pr found nothing recordable, so the real reviewer went + # uncredited and the declining one stayed marked busy. + # + # pr.requested_reviewers alone is not enough, because GitHub drops a + # reviewer from it the moment they submit; the request event persists. + authorised = reviewer in self._current_reviewers(pr) or any( + event.event == "review_requested" + and getattr(event, "requested_reviewer", None) + and event.requested_reviewer.login.lower() == reviewer + for event in pr.as_issue().get_events() + ) + if not authorised: print("The reviewer is not the one who agreed to review the PR.") sys.exit() pr.create_issue_comment( @@ -572,6 +500,8 @@ def approve_reviewer(self): ) def get_user_info(self, user): + if user is None: + return "", "" user_id = user.login user_name = user.name or user_id return user_id.lower(), user_name @@ -606,17 +536,22 @@ def closed_pr(self): pr = self.repo.get_pull(int(self.pr_number)) reviews = pr.get_reviews().reversed merged = os.getenv("MERGED_STATUS") == "true" - if merged and reviews.totalCount > 0: - comment_reviewers = self._search_comment_user(pr, "for the review") - the_review = next( - ( - review - for review in reviews - if comment_reviewers - and review.user.login.lower() == comment_reviewers[0] - ), - reviews[0], - ) + # What may be recorded as *the* review of this PR, best first. A + # verdict from the appointed reviewer wins; failing that, any review + # they left, because a reviewer who clicks Comment instead of Approve + # has still done the work and must still be credited and released. + # A passer-by's review is never recorded, whatever its state. + # Only somebody actually on the hook may be recorded: the appointed + # reviewer, whoever GitHub still lists as requested, or whoever the bot + # already thanked. Anyone can approve a PR on a public repo, so a + # passer-by's verdict must never be credited with the review. + thanked = self._search_comment_user(pr, "for the review") + eligible = self._current_reviewers(pr) | set(thanked[:1]) + by_eligible = self._reviews_by(pr, eligible) + candidates = [r for r in by_eligible if r.state in self.VERDICT_STATES] + candidates += [r for r in by_eligible if r not in candidates] + if merged and candidates: + the_review = candidates[0] open_date = pr.created_at.strftime("%Y-%m-%d") submitted_at = datetime.now(tz=cet).strftime("%Y-%m-%d") @@ -625,7 +560,10 @@ def closed_pr(self): reviewer_id, reviewer_name = self.get_user_info(the_review.user) interaction_count = 0 - other_participants = set() + # Keyed by lower-cased GitHub ID so the roster lookup below can + # match it. The value is the display name, used only as a + # fallback when the person is not on the roster. + other_participants = {} for comment in list(pr.get_issue_comments()) + list(reviews): body = comment.body.strip() if not body: @@ -643,11 +581,11 @@ def closed_pr(self): supervisor_id, reviewer_id, }: - other_participants.add(comment_user_name) + other_participants[comment_user_id] = comment_user_name supervisor_institution = "" reviewer_institution = "" - for entry in self.EAR_reviewer.data: + for entry in self.roster.data: github_id = entry.get("Github ID", "").lower() full_name = entry.get("Full Name") if full_name: @@ -658,8 +596,7 @@ def closed_pr(self): if github_id == reviewer_id: reviewer_name = full_name if github_id in other_participants: - other_participants.remove(github_id) - other_participants.add(full_name) + other_participants[github_id] = full_name if github_id == supervisor_id: supervisor_institution = entry.get("Institution", "") if github_id == reviewer_id: @@ -668,50 +605,101 @@ def closed_pr(self): species = self._search_in_body(pr, "Species") tag = self._search_in_body(pr, "Project") researcher_institution = self._search_for_institution(pr) - other_participants_str = ", ".join(sorted(other_participants)) - - self.EAR_reviewer.add_pr( - pr_url=pr.html_url, - species=species, - tag=tag, - requester_name=researcher_name, - requester_affiliation=researcher_institution, - reviewer_name=reviewer_name, - reviewer_affiliation=reviewer_institution, - supervisor_name=supervisor_name, - supervisor_affiliation=supervisor_institution, - interaction_count=interaction_count, - open_date=open_date, - approval_date=submitted_at, - other_participants=other_participants_str, - notes="N/A", - ) + other_participants_str = ", ".join(sorted(other_participants.values())) + # Resolved before any commit below. This used to run after both + # CSVs were already written, so a merged PR with no PDF left the + # data half-updated and could not be re-run safely. + EAR_pdf = next( + ( + file + for file in pr.get_files() + if file.filename.lower().endswith(".pdf") + ), + None, + ) institution = self._search_for_institution(pr) - self.EAR_reviewer.update_reviewers_list( + + # Every precondition is checked before the first write, so a run + # that cannot record the review writes nothing at all. + # + # It deliberately does not release the reviewer either. WF6 can + # fire again if the PR is reopened and re-closed, and an automatic + # release would decrement Working PRs once per run. Retaining the + # count and asking for CLEAR matches what the bot already does for + # a PR closed unmerged, and CLEAR is issued by a human once. + problem = None + if EAR_pdf is None: + problem = "I could not find an EAR PDF in this PR" + elif self.roster.missing([reviewer_id]): + problem = ( + f"the reviewer (`{reviewer_id or 'unknown'}`) is not on the" + " reviewers list" + ) + if problem: + pr.create_issue_comment( + f"I did not record this review because {problem}.\n" + "The reviewers keep their working PR count for now; if that" + " is not right, please instruct me to clear the active tasks." + ) + pr.add_to_labels("ERROR!") + print(f"Nothing recorded for PR #{pr.number}: {problem}.") + return + + recorded = self.roster.record_review( + row_values=[ + pr.html_url, + species, + tag, + researcher_name, + researcher_institution, + reviewer_name, + reviewer_institution, + supervisor_name, + supervisor_institution, + interaction_count, + open_date, + submitted_at, + other_participants_str, + "N/A", + ], reviewers=[reviewer_id], - busy=False, institution=institution, submitted_at=submitted_at, ) - EAR_pdf = next( - file - for file in pr.get_files() - if file.filename.lower().endswith(".pdf") - ) - self._add_yaml_file(EAR_pdf.filename) + # A dedup hit means an earlier run logged the review, but it may + # have died before finishing here. Returning outright left the + # YAML permanently ungenerated, so pick up whatever is still + # missing instead. The Slack post is not retried: it has no + # idempotency key, and re-announcing an assembly to the whole + # consortium is worse than a human reposting a missed one. + yaml_path = self._yaml_path_for(EAR_pdf.filename) + if recorded or not roster_module.exists(self.repo, yaml_path): + self._add_yaml_file(EAR_pdf.filename) + if not recorded: + print( + f"{pr.html_url} was already recorded; skipping the Slack post." + ) + return if self._search_in_body(pr, "Project") == "ERGA-BGE": EAR_pdf_url = re.sub(r"/blob/[\w\d]+/", "/blob/main/", EAR_pdf.blob_url) slack_post = ( f":tada: *New Assembly Finished!* :tada:\n\n" - f"Congratulations to {researcher_name} and the {institution} team for the high-quality assembly of _{species}_\n\n" - f"The assembly was reviewed by {reviewer_name}, and the process supervised by {supervisor_name}. The EAR can be found in the following link:\n" + f"Congratulations to {self._slack_escape(researcher_name)} and the" + f" {self._slack_escape(institution)} team for the high-quality" + f" assembly of _{self._slack_escape(species)}_\n\n" + f"The assembly was reviewed by {self._slack_escape(reviewer_name)}," + f" and the process supervised by {self._slack_escape(supervisor_name)}." + " The EAR can be found in the following link:\n" f"{EAR_pdf_url}" ) self._create_slack_post(slack_post) elif not merged: - supervisor = pr.assignee.login + # A PR can be closed before a supervisor is ever assigned, and the + # EAR-UPDATE path never assigns one at all. Fall back to the + # researcher so this warning still reaches somebody. + supervisor = pr.assignee.login if pr.assignee else pr.user.login pr.create_issue_comment( f"Attention @{supervisor}!\n" "The PR has been closed, but the reviewers will retain their working PR count in case it is re-opened.\n" @@ -719,17 +707,174 @@ def closed_pr(self): ) pr.add_to_labels("ERROR!") else: + # A merged PR with no recordable review, for example an EAR-UPDATE. + # The default matters: a bare next() raised StopIteration here and + # the job died with a traceback and no comment on the PR. EAR_pdf_filename = next( - file.filename - for file in pr.get_files() - if file.filename.lower().endswith(".pdf") + ( + file.filename + for file in pr.get_files() + if file.filename.lower().endswith(".pdf") + ), + None, ) + if EAR_pdf_filename is None: + pr.create_issue_comment( + "This PR was merged without an EAR PDF, so there was nothing for me to update." + ) + pr.add_to_labels("ERROR!") + print("No PDF file found in this merged PR.") + return self._add_yaml_file(EAR_pdf_filename) print("No review has been found for this merged PR.") + if self._current_reviewers(pr): + # Somebody was appointed but nothing they wrote is recordable, + # so the merge cannot credit them and their Working PRs stays + # up. The two sibling unrecordable paths both warn and label; + # staying silent here left a reviewer permanently busy with + # nobody told. + supervisor = pr.assignee.login if pr.assignee else pr.user.login + pr.create_issue_comment( + f"Attention @{supervisor}! This PR was merged, but I could not" + " find a review from the appointed reviewer, so I did not record" + " it. They keep their working PR count; if that is not right," + " please instruct me to clear the active tasks." + ) + pr.add_to_labels("ERROR!") pr.create_issue_comment( "The YAML file has been updated based on the new EAR.pdf" ) + @staticmethod + def _yaml_path_for(pdf_filename): + """The YAML path for an EAR PDF. + + splitext, not replace(".pdf", ".yaml"): the finder that selects the + PDF matches case-insensitively, so a file committed as ``*.PDF`` left + the derived name identical to the PDF's own path. That silently + skipped generation on one branch and, on the other, read the binary + PDF as text and overwrote it. + """ + return os.path.splitext(pdf_filename)[0] + ".yaml" + + @staticmethod + def _unquoted_lines(comment_text): + """The comment's own lines, with any quoted reply stripped out. + + Quoting the bot's own message back at it must not count as a command, + which is what matching against the raw body allowed. + """ + return [ + stripped + for line in comment_text.split("\n") + if (stripped := line.strip()) and not stripped.startswith(">") + ] + + @classmethod + def _says(cls, comment_text, pattern): + """True if any line the author actually wrote matches ``pattern``. + + Deliberately not first-line-only: people open with a greeting, and + rejecting that was worse than the loose substring match it replaced, + because the failure path stamps ERROR! and fails the workflow. + """ + return any( + re.search(pattern, line, re.IGNORECASE) + for line in cls._unquoted_lines(comment_text) + ) + + @classmethod + def _decision(cls, comment_text, options): + """Pick between competing answers by which the author wrote first. + + ``options`` maps a name to a pattern. A line answers only if exactly + one option matches it; a line matching both is ambiguous and is + skipped rather than guessed at. + + Guessing was tried and was worse. Taking the leftmost match turned + "I can't say yes or no until Monday" into an acceptance, appointing a + reviewer who had explicitly not committed. The case that motivated it + -- "Yes, no problem." being rejected -- is handled by the NO pattern + excluding the "no problem/worries/..." idioms instead, which is the + real distinction. + """ + for line in cls._unquoted_lines(comment_text): + hits = [ + name + for name, pattern in options.items() + if re.search(pattern, line, re.IGNORECASE) + ] + if len(hits) == 1: + return hits[0] + return None + + # DISMISSED is included: branch protection flips an approval to DISMISSED + # when new commits land, and that review still represents work done. + # Excluding it meant a merged PR whose approval had been dismissed was + # never recorded at all. + VERDICT_STATES = ("APPROVED", "CHANGES_REQUESTED", "DISMISSED") + + @classmethod + def _binding_reviews(cls, pr): + """Reviews that carry a verdict, newest first. + + get_reviews() also returns COMMENTED reviews, which any user can + leave on a public repo and which cannot be deleted afterwards. + Counting those meant a single stray comment review stopped the bot + from ever assigning a reviewer, and could be recorded as the review + of the PR once it merged. + """ + return [ + review + for review in pr.get_reviews().reversed + if review.state in cls.VERDICT_STATES + ] + + def _current_reviewers(self, pr): + """Whoever is on the hook right now, lower-cased. + + Only the most recent person asked, not the whole ask history: earlier + names in that list are reviewers who already timed out, and treating + their old review as live work blocked the PR forever. Anyone GitHub + still lists as a requested reviewer counts too, which covers PRs + assigned by hand rather than by the bot. + """ + asked = self._search_comment_user(pr, "do you agree to review") + current = {asked[0]} if asked else set() + return current | { + user.login.lower() for user in pr.requested_reviewers if user + } + + def _reviews_by(self, pr, logins): + """Reviews by any of ``logins``, newest first. + + The ordering matches _binding_reviews deliberately: closed_pr takes + candidates[0] as the review to record, so a chronological list here + would credit the oldest verdict rather than the latest one. + """ + return [ + review + for review in pr.get_reviews().reversed + if review.user and review.user.login.lower() in logins + ] + + def _review_in_progress(self, pr): + """True if somebody is already reviewing, so the bot must not re-solicit. + + Author-aware on purpose. GitHub clears the pending review request as + soon as a requested reviewer submits *any* review, including a + comment-only one, so filtering purely on review state let the bot + declare "Time is out!" and hand the PR to somebody else while the + appointed reviewer was mid-review. + """ + # Strictly author-based. An earlier version also accepted a verdict + # from anyone at all as long as *somebody* was appointed, which let a + # passer-by's CHANGES_REQUESTED on a public repo freeze the PR: the + # deadline never fired, no successor was asked, and the appointed + # reviewer's eventual "Yes" was discarded. + current = self._current_reviewers(pr) + return bool(current) and bool(self._reviews_by(pr, current)) + def _is_bot_user(self, comment): user_type = comment.raw_data.get("user", {}).get("type", None) return str(user_type).lower() == "bot" @@ -834,6 +979,20 @@ def _deadline(self, start_date): current_date += time_to_add return current_date + @staticmethod + def _slack_escape(text): + """Escape the three characters Slack treats as markup. + + Without this a species or institution name taken from the PR body + could contain and notify the whole channel. + """ + return ( + str(text) + .replace("&", "&") + .replace("<", "<") + .replace(">", ">") + ) + def _create_slack_post(self, content): from slack_sdk import WebClient @@ -864,10 +1023,10 @@ def _add_yaml_file(self, EAR_pdf_filename): capture_output=True, text=True, ) - yaml_filename = EAR_pdf_filename.replace(".pdf", ".yaml") + yaml_filename = self._yaml_path_for(EAR_pdf_filename) with open(_get_local_path(yaml_filename), "r") as file: yaml_content = file.read() - commit(self.repo, yaml_filename, "Add YAML file", yaml_content) + replace(self.repo, yaml_filename, "Add YAML file", yaml_content) print(output_pdf_to_yaml.stdout, output_pdf_to_yaml.stderr) diff --git a/ear_bot/requirements.txt b/ear_bot/requirements.txt index 314ebbb4..d4d05428 100644 --- a/ear_bot/requirements.txt +++ b/ear_bot/requirements.txt @@ -3,3 +3,4 @@ pytz slack_sdk pdfplumber pyyaml +requests diff --git a/ear_bot/roster.py b/ear_bot/roster.py new file mode 100644 index 00000000..c12527fa --- /dev/null +++ b/ear_bot/roster.py @@ -0,0 +1,320 @@ +"""Roster and review-log storage for the EAR bot. + +Everything that reads or writes the two CSV files in ``rev/`` lives here: + + rev/reviewers_list.csv the reviewer roster and its scores + rev/EAR_reviews.csv the append-only log of completed reviews + +Two rules this module exists to enforce: + +1. **A write never silently loses another run's changes.** ``update_if_unchanged`` + sends the blob SHA the content was based on, so GitHub rejects the write if + the file moved underneath us. On rejection the caller's data is re-read and + the change is re-applied, rather than clobbering or giving up. + +2. **Recording a completed review is all-or-nothing.** ``record_review`` + validates every precondition before writing anything, so a merge is either + fully recorded or not recorded at all. It writes the review-log row before + the roster counters, because that row is the key a re-run checks: getting + only as far as the row leaves a reviewer uncredited and visibly still busy, + whereas applying the counters first and failing leaves nothing to show it + happened, so a re-run applies them again and corrupts scores silently. +""" + +import csv +import io + +from github import UnknownObjectException + +from rev import get_EAR_reviewer + +REVIEWERS_CSV = "rev/reviewers_list.csv" +EAR_REVIEWS_CSV = "rev/EAR_reviews.csv" + +# How many times to re-read and re-apply after a conflicting write. +MAX_WRITE_ATTEMPTS = 3 + + +class RosterError(Exception): + """A precondition for updating the roster was not met.""" + + +def replace(repo, path, message, content): + """Write ``path`` unconditionally, creating it if absent. + + For files only this bot produces, where there is no concurrent writer to + conflict with. Use ``update_if_unchanged`` for the shared CSVs. + """ + try: + contents = repo.get_contents(path) + if isinstance(contents, list): + raise RosterError(f"Expected a file, got a directory: {path}") + result = repo.update_file(path, message, content, contents.sha) + print(f"Updated {path} file.") + except UnknownObjectException: + result = repo.create_file(path, message, content) + print(f"Created {path} file.") + return result["content"].sha + + +def update_if_unchanged(repo, path, message, content, sha): + """Write ``path`` only if its current blob still matches ``sha``. + + ``sha`` is required: an optional one would make forgetting it re-introduce + the lost-update race this function exists to prevent. Returns the new blob + SHA so a caller writing the same path repeatedly can carry it forward. + """ + result = repo.update_file(path, message, content, sha) + print(f"Updated {path} file.") + return result["content"].sha + + +def exists(repo, path): + """True if ``path`` is already committed.""" + try: + repo.get_contents(path) + return True + except UnknownObjectException: + return False + + +def _fetch(repo, path): + contents = repo.get_contents(path) + if isinstance(contents, list): + raise RosterError(f"Expected a file, got a directory: {path}") + text = contents.decoded_content.decode("utf-8") + if not text: + raise RosterError(f"The CSV file is empty: {path}") + return text, contents.sha + + +class Roster: + """The reviewer roster, plus the review log it is updated alongside.""" + + def __init__(self, repo): + self.repo = repo + self._load() + + def _load(self): + text, self.sha = _fetch(self.repo, REVIEWERS_CSV) + self._committed_text = text + self.fieldnames = get_EAR_reviewer.csv_fieldnames(text) + self.data = get_EAR_reviewer.parse_csv(text) + + def ids(self): + return {row.get("Github ID", "").lower() for row in self.data} + + def missing(self, reviewers): + """Which of ``reviewers`` are not on the roster. + + A blank ID counts as missing. get_user_info() returns "" for a + deleted GitHub account, and treating that as known let a merge be + half-recorded: the log row was written with empty reviewer fields + while the roster update quietly did nothing. + """ + known = self.ids() + return {r for r in reviewers if not r or r.lower() not in known} + + def _write(self, message, mutate): + """Apply ``mutate`` to the roster rows and commit, retrying on conflict. + + ``mutate`` is a callable taking the row list and changing it in place. + It is deliberately re-run rather than re-sent: on a conflict the local + copy is discarded, the file is re-read, and the same change is applied + to the *other run's* committed rows. Re-sending our own snapshot would + overwrite whatever they wrote, which is the lost update this class + exists to prevent. + + Known limitation: if our write lands, the response is lost, *and* + another run commits on top before we re-read, the content check below + cannot tell that ours landed and the change is applied twice. Closing + that needs a per-operation idempotency key, which the roster CSV has + no column for -- unlike the review log, which dedupes on the PR URL. + """ + for attempt in range(MAX_WRITE_ATTEMPTS): + mutate(self.data) + payload = get_EAR_reviewer.format_csv(self.data, self.fieldnames) + try: + self.sha = update_if_unchanged( + self.repo, REVIEWERS_CSV, message, payload, self.sha + ) + return + except Exception as exc: + # Drop our uncommitted edits either way, so the caller is never + # left holding counters that were never written. + self._load() + # An exception does not prove the write was rejected. GitHub + # may have committed it and only the response been lost, and + # re-running `mutate` on a file that already contains our + # change would apply it twice -- the silent score corruption + # this class exists to prevent. Only a stale SHA proves + # nothing landed, and the cheap way to tell them apart is to + # look at what is actually committed now. + if self._committed_text.strip() == payload.strip(): + print("Roster write had already landed; not re-applying.") + return + if attempt == MAX_WRITE_ATTEMPTS - 1: + raise + print(f"Roster write rejected ({exc}); re-reading and retrying.") + + def apply( + self, + reviewers, + busy, + institution="", + submitted_at="", + fined_reviewers=(), + message="Update reviewers list", + strict=True, + ): + """Adjust counters for ``reviewers`` and commit. + + With ``strict`` (the default) an unknown ID raises before anything is + touched, so a caller recording a review cannot half-apply a change set. + + Release paths pass ``strict=False``: freeing everyone else's counters + matters more than refusing because one person has since left the + consortium and been removed from the roster. Without this, a single + departed reviewer permanently blocks the CLEAR command for that PR. + """ + # Checked before blanks are dropped, so a deleted account (which + # get_user_info reports as "") is reported rather than silently + # becoming a no-op. + unknown = self.missing(reviewers) + reviewers = {r.lower() for r in reviewers if r} + if unknown: + named = sorted(u or "(unknown user)" for u in unknown) + if strict: + raise RosterError(f"Not on the reviewers list: {', '.join(named)}") + print(f"Not on the reviewers list, skipping: {', '.join(named)}") + reviewers -= {u.lower() for u in unknown if u} + + # Not gated on `reviewers`: the timeout penalty is a separate change + # set that happens to travel with it. A reviewer who was asked twice + # and then accepted leaves `reviewers` empty while still owing the + # penalty, and returning early here dropped it silently. + # missing() returns IDs in their original case, so it has to be + # lower-cased before subtracting, exactly as the reviewers set above. + fined = {r.lower() for r in fined_reviewers if r} - { + u.lower() for u in self.missing(fined_reviewers) if u + } + if not reviewers and not fined: + print("No reviewers to update.") + return + + def mutate(rows): + for row in rows: + row_id = row.get("Github ID", "").lower() + score = int(row.get("Calling Score", 1000) or 1000) + total = int(row.get("Total Reviews", 0) or 0) + working = int(row.get("Working PRs", 0) or 0) + + if row_id in reviewers: + row["Working PRs"] = str( + working + 1 if busy else max(0, working - 1) + ) + if submitted_at: + score -= 1 + row["Calling Score"] = str(score) + row["Total Reviews"] = str(total + 1) + row["Last Review"] = submitted_at + # Not an elif: a reviewer who timed out is always also in + # `reviewers`, since both sets come from the same "do you agree + # to review" comments. + if row_id in fined: + score += 1 + row["Calling Score"] = str(score) + if ( + institution + and row.get("Institution", "").lower() == institution.lower() + ): + row["Calling Score"] = str(score + 1) + + self._write(message, mutate) + print(f"Updated the reviewers list for {', '.join(sorted(reviewers))}.") + + def already_recorded(self, pr_url): + """True if the review log already has a row for this PR. + + WF6 fires once per close, but a manual re-run would otherwise append a + duplicate row for the same PR. + """ + text, _ = _fetch(self.repo, EAR_REVIEWS_CSV) + return any( + row and row[0].strip() == pr_url + for row in csv.reader(io.StringIO(text)) + ) + + def record_review(self, row_values, reviewers, institution, submitted_at): + """Append a review row and update the roster, or do neither. + + Every precondition is checked before the first write. The log row is + written *first* because it is the idempotency key: if the roster write + then fails, a re-run sees the row and stops, leaving a reviewer + uncredited, which is visible as a stuck Working PRs count. The + reverse order fails the other way -- the counters are applied with no + row to record that it happened, so a re-run applies them again and + corrupts scores silently. Do not swap these. + + Returns False without touching the counters if the review was already + logged, including when a concurrent run logged it between our check + and our write. + """ + pr_url = row_values[0] + if self.already_recorded(pr_url): + print(f"{pr_url} is already in the review log; nothing to record.") + return False + + unknown = self.missing(reviewers) + if unknown: + raise RosterError( + f"Not on the reviewers list: {', '.join(sorted(unknown or {'(unknown user)'}))}" + ) + + if not self._append_review(row_values): + # Another run won the race and logged it. It applies the counters + # too, so applying them here as well would double-count. + print(f"{pr_url} was logged by a concurrent run; leaving counters to it.") + return False + self.apply( + reviewers=reviewers, + busy=False, + institution=institution, + submitted_at=submitted_at, + ) + print(f"Recorded the review for {pr_url}.") + return True + + def _append_review(self, row_values): + """Append one row to the review log, retrying on conflict. + + Re-reads on every attempt, so a row another run appended in the + meantime is preserved rather than overwritten. Returns True if this + call wrote the row and False if it was already there, so the caller + can tell "recorded" from "somebody else recorded it". + """ + pr_url = row_values[0] + for attempt in range(MAX_WRITE_ATTEMPTS): + text, sha = _fetch(self.repo, EAR_REVIEWS_CSV) + if any( + row and row[0].strip() == pr_url + for row in csv.reader(io.StringIO(text)) + ): + return False + buffer = io.StringIO() + csv.writer(buffer, lineterminator="\n").writerow(row_values) + if not text.endswith("\n"): + text += "\n" + try: + update_if_unchanged( + self.repo, + EAR_REVIEWS_CSV, + "Add new EAR review", + text + buffer.getvalue(), + sha, + ) + return True + except Exception as exc: + if attempt == MAX_WRITE_ATTEMPTS - 1: + raise + print(f"Review log write rejected ({exc}); re-reading and retrying.") diff --git a/ear_bot/tests/__init__.py b/ear_bot/tests/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/ear_bot/tests/conftest.py b/ear_bot/tests/conftest.py new file mode 100644 index 00000000..9850a27f --- /dev/null +++ b/ear_bot/tests/conftest.py @@ -0,0 +1,246 @@ +"""Fakes standing in for the GitHub API, so the bot can be tested offline. + +Only the surface the bot actually touches is modelled. The important one is +FakeRepo, which enforces GitHub's real contract for update_file: the blob SHA +you pass must match the current one, or the write is rejected. That is what +makes the concurrency tests meaningful. +""" + +import csv +import datetime as dt +import io +import os +import sys + +import pytest +from github import UnknownObjectException + +sys.path.insert(0, os.path.abspath(os.path.join(os.path.dirname(__file__), "..", ".."))) + +REVIEWERS_CSV = "rev/reviewers_list.csv" +EAR_REVIEWS_CSV = "rev/EAR_reviews.csv" + +HEADER = ( + "Github ID,Full Name,Institution,Supervisor,Total Reviews," + "Last Review,Active,Working PRs,Calling Score" +) +ROWS = ( + "alice,Alice A,Sanger,N,3,2025-01-01,Y,1,1000\n" + "bob,Bob B,CNAG,N,1,2025-02-02,Y,0,1000\n" + "carol,Carol C,Genoscope,Y,2,2025-03-03,Y,0,1000\n" +) + + +class Conflict(Exception): + """Stands in for the 409 PyGithub raises on a stale SHA.""" + + +class FakeContents: + def __init__(self, text, sha, path): + self.decoded_content = text.encode() + self.sha = sha + self.path = path + + +class FakeRepo: + def __init__(self, roster=None, reviews="PR URL,Species\n"): + self.files = { + REVIEWERS_CSV: [roster or f"{HEADER}\n{ROWS}", "roster-0"], + EAR_REVIEWS_CSV: [reviews, "reviews-0"], + } + self.writes = [] + self._n = 0 + # Callables invoked once, just before a write to that path, to + # simulate another workflow run committing underneath us. + self.before_write = {} + + def get_contents(self, path): + if path not in self.files: + raise UnknownObjectException(404, "Not Found", None) + text, sha = self.files[path] + return FakeContents(text, sha, path) + + def update_file(self, path, message, content, sha): + hook = self.before_write.pop(path, None) + if hook: + hook(self) + if sha != self.files[path][1]: + raise Conflict(f"409: have {self.files[path][1]}, got {sha}") + self._n += 1 + self.files[path] = [content, f"{path}-{self._n}"] + self.writes.append((path, content)) + return {"content": FakeContents(content, self.files[path][1], path)} + + def create_file(self, path, message, content): + self._n += 1 + self.files[path] = [content, f"{path}-{self._n}"] + self.writes.append((path, content)) + return {"content": FakeContents(content, self.files[path][1], path)} + + # -- helpers for assertions ------------------------------------------ + def roster_rows(self): + text = self.files[REVIEWERS_CSV][0] + return {r["Github ID"]: r for r in csv.DictReader(io.StringIO(text.strip()))} + + def review_rows(self): + text = self.files[EAR_REVIEWS_CSV][0] + return [r for r in csv.reader(io.StringIO(text.strip())) if r] + + def commit_concurrently(self, path, new_text): + """Register a one-shot concurrent commit to ``path``.""" + + def hook(repo): + repo._n += 1 + repo.files[path] = [new_text, f"{path}-concurrent-{repo._n}"] + + self.before_write[path] = hook + + +class FakeUser: + def __init__(self, login, name=None): + self.login = login + self.name = name + + +class FakeReview: + def __init__(self, login, state): + self.user = FakeUser(login) if login is not None else None + self.state = state + + +class FakeReviews(list): + @property + def totalCount(self): + return len(self) + + @property + def reversed(self): + return FakeReviews(list(self)[::-1]) + + +class FakeComment: + def __init__(self, body, bot=True, login="erga-ear-bot[bot]"): + self.body = body + self.raw_data = {"user": {"type": "Bot" if bot else "User"}} + self.user = FakeUser(login) + self.created_at = dt.datetime.now(dt.timezone.utc) + + +class FakeComments(list): + @property + def reversed(self): + return FakeComments(list(self)[::-1]) + + +class FakeEvent: + def __init__(self, event, requested_reviewer=None): + self.event = event + self.requested_reviewer = ( + FakeUser(requested_reviewer) if requested_reviewer else None + ) + + +class FakeIssue: + def __init__(self, events): + self._events = events + + def get_events(self): + return self._events + + +class FakePR: + def __init__( + self, + number=1, + author="researcher", + assignee="supervisor", + reviews=(), + comments=(), + requested=(), + events=(), + labels=("ERGA-BGE",), + body="- Project: ERGA-BGE\n- Affiliation: Sanger\n- Species: Foo\n", + state="open", + files=("Assembly_Reports/x/x_EAR.pdf",), + ): + self.number = number + self.user = FakeUser(author) + self.assignee = FakeUser(assignee) if assignee else None + self.assignees = [self.assignee] if assignee else [] + self.html_url = f"https://github.com/o/r/pull/{number}" + self._reviews = FakeReviews(reviews) + self._comments = FakeComments(comments) + self.requested_reviewers = [FakeUser(r) for r in requested] + self._events = list(events) + self._labels = list(labels) + self.body = body + self.state = state + self._files = list(files) + self.created_at = dt.datetime.now(dt.timezone.utc) + self.comments = [] + self.labels_added = [] + self.review_requests = [] + + def get_reviews(self): + return self._reviews + + def get_issue_comments(self): + return self._comments + + def get_labels(self): + return [type("L", (), {"name": n})() for n in self._labels] + + def get_review_requests(self): + return ( + type("P", (), {"totalCount": len(self.requested_reviewers)})(), + type("P", (), {"totalCount": 0})(), + ) + + def get_files(self): + return [ + type("F", (), {"filename": f, "blob_url": f"https://x/{f}", "status": "added"})() + for f in self._files + ] + + def as_issue(self): + return FakeIssue(self._events) + + def create_issue_comment(self, body): + self.comments.append(body) + + def add_to_labels(self, label): + self.labels_added.append(label) + + def remove_from_labels(self, label): + if label in self._labels: + self._labels.remove(label) + + def add_to_assignees(self, user): + self.assignees.append(FakeUser(user)) + + def create_review_request(self, users): + self.review_requests.extend(users) + + +@pytest.fixture +def repo(): + return FakeRepo() + + +@pytest.fixture +def roster(repo): + from ear_bot.roster import Roster + + return Roster(repo) + + +@pytest.fixture +def bot(): + """An EARBotReviewer with __init__ bypassed, since it does network I/O.""" + from ear_bot.ear_bot_reviewer import EARBotReviewer + + b = object.__new__(EARBotReviewer) + b.valid_projects = ["ERGA-BGE", "ERGA-Pilot", "ERGA-Community"] + b.repo = None + b.roster = None + return b diff --git a/ear_bot/tests/test_bot.py b/ear_bot/tests/test_bot.py new file mode 100644 index 00000000..42157c57 --- /dev/null +++ b/ear_bot/tests/test_bot.py @@ -0,0 +1,226 @@ +"""Comment parsing, review-state detection, and the merge path.""" + +from ear_bot.ear_bot_reviewer import EARBotReviewer + +from .conftest import FakeComment, FakePR, FakeEvent, FakeReview + + +# --- comment parsing -------------------------------------------------------- + +def test_okay_is_accepted(bot): + assert bot._says("Okay", r"\bok(ay)?\b") + + +def test_greeting_before_ok_is_accepted(bot): + assert bot._says("Hi Diego,\n\nOK, happy to supervise.", r"\bok(ay)?\b") + + +def test_ok_inside_another_word_is_not_a_confirmation(bot): + for text in ("Looks good to me", "I took a look", "this is broken"): + assert not bot._says(text, r"\bok(ay)?\b"), text + + +def test_quoting_the_bot_is_not_a_confirmation(bot): + quoted = "> Please reply to this message only with **OK**\nI cannot supervise this one" + assert not bot._says(quoted, r"\bok(ay)?\b") + + +def test_clear_is_recognised_on_a_later_line(bot): + text = "Hi bot, this one is abandoned.\n@erga-ear-bot CLEAR" + assert bot._says(text, r"@erga-ear-bot\s+clear") + + +def test_quoted_clear_is_ignored(bot): + text = "> please instruct me with @erga-ear-bot clear\nLeaving it open for now" + assert not bot._says(text, r"@erga-ear-bot\s+clear") + + +OPTS = EARBotReviewer.YES_NO + + +def test_decline_mentioning_yes_later_is_still_a_decline(bot): + """The regression that made the bot appoint someone who had just refused.""" + text = "No, sorry I can't.\nMaybe ask @alice - yes, she knows this genus." + assert bot._decision(text, OPTS) == "no" + + +def test_greeting_before_yes_is_an_acceptance(bot): + assert bot._decision("Hi!\nYes, happy to review.", OPTS) == "yes" + + +def test_plain_answers_still_work(bot): + assert bot._decision("Yes", OPTS) == "yes" + assert bot._decision("No", OPTS) == "no" + + +def test_no_answer_at_all_returns_none(bot): + assert bot._decision("I will look at it next week", OPTS) is None + + +# --- review state ----------------------------------------------------------- + +def asked(login): + return FakeComment(f"Hi @{login}, do you agree to review this assembly?") + + +def test_appointed_reviewers_comment_review_blocks_resolicitation(bot): + """GitHub clears the review request once they submit anything.""" + pr = FakePR(reviews=[FakeReview("rev", "COMMENTED")], comments=[asked("rev")]) + assert bot._review_in_progress(pr) + + +def test_passerby_comment_review_does_not_block(bot): + pr = FakePR(reviews=[FakeReview("passerby", "COMMENTED")], comments=[asked("rev")]) + assert not bot._review_in_progress(pr) + + +def test_passerby_verdict_does_not_block_when_nobody_is_appointed(bot): + """A stranger must not be able to freeze a PR on a public repo.""" + pr = FakePR(reviews=[FakeReview("passerby", "CHANGES_REQUESTED")], comments=[]) + assert not bot._review_in_progress(pr) + + +def test_timed_out_reviewers_old_review_does_not_block_forever(bot): + """Only the person currently on the hook counts, not the whole history.""" + pr = FakePR( + reviews=[FakeReview("first", "COMMENTED")], + comments=[asked("first"), asked("second")], # newest first after .reversed + ) + # _search_comment_user reads newest-first, so "second" is current. + assert not bot._review_in_progress(pr) + + +def test_hand_assigned_reviewer_blocks(bot): + pr = FakePR(reviews=[FakeReview("manual", "COMMENTED")], comments=[], requested=["manual"]) + assert bot._review_in_progress(pr) + + +def test_dismissed_approval_still_counts_as_a_verdict(bot): + pr = FakePR(reviews=[FakeReview("rev", "DISMISSED")], comments=[asked("rev")]) + assert len(bot._binding_reviews(pr)) == 1 + + +def test_deleted_account_review_does_not_crash(bot): + """review.user is None when the author deleted their account.""" + pr = FakePR(reviews=[FakeReview(None, "APPROVED")], comments=[asked("rev")]) + assert bot._reviews_by(pr, {"rev"}) == [] + bot._review_in_progress(pr) # must not raise + + +# --- approve_reviewer authorisation ---------------------------------------- + +def run_approve(bot, pr, approver): + bot.pr_number = "1" + bot.reviewer = approver + bot.repo = type("R", (), {"get_pull": staticmethod(lambda n: pr)})() + try: + bot.approve_reviewer() + except SystemExit: + pass + return pr.comments + + +def test_outsider_approval_is_not_thanked(bot): + pr = FakePR(reviews=[FakeReview("outsider", "APPROVED")], comments=[asked("rev")]) + assert run_approve(bot, pr, "outsider") == [] + + +def test_appointed_reviewer_is_thanked(bot): + pr = FakePR(reviews=[FakeReview("rev", "APPROVED")], comments=[asked("rev")]) + assert any("for the review" in c for c in run_approve(bot, pr, "rev")) + + +def test_hand_assigned_reviewer_is_thanked(bot): + """requested_reviewers is empty by then; the request event still records it.""" + pr = FakePR( + reviews=[FakeReview("manual", "APPROVED")], + comments=[], + requested=[], + events=[FakeEvent("review_requested", "manual")], + ) + assert any("for the review" in c for c in run_approve(bot, pr, "manual")) + + +def test_stranger_without_a_request_event_is_not_thanked(bot): + pr = FakePR( + reviews=[FakeReview("stranger", "APPROVED")], + comments=[], + events=[FakeEvent("labeled")], + ) + assert run_approve(bot, pr, "stranger") == [] + + +# --- what may be recorded as the review ------------------------------------ + +def test_passerby_verdict_is_not_recordable(bot): + """Anyone can approve on a public repo; only the appointed reviewer counts.""" + pr = FakePR(reviews=[FakeReview("passerby", "APPROVED")], comments=[asked("rev")]) + eligible = bot._current_reviewers(pr) | set( + bot._search_comment_user(pr, "for the review")[:1] + ) + assert bot._reviews_by(pr, eligible) == [] + + +def test_appointed_reviewers_comment_review_is_recordable(bot): + """Clicking Comment instead of Approve still earns the credit.""" + pr = FakePR(reviews=[FakeReview("rev", "COMMENTED")], comments=[asked("rev")]) + eligible = bot._current_reviewers(pr) + assert len(bot._reviews_by(pr, eligible)) == 1 + + +def test_no_problem_is_agreement_not_refusal(bot): + """These are ordinary acceptances; reading them as declines reassigned + the assembly to somebody else.""" + assert bot._decision("Yes, no problem.", OPTS) == "yes" + assert bot._decision("No problem - yes I'll take it", OPTS) == "yes" + for text in ("Sure, no problem", "No worries, I can review it"): + assert bot._decision(text, OPTS) != "no", text + + +def test_genuinely_ambiguous_replies_are_not_guessed(bot): + """Guessing here appointed reviewers who had explicitly not committed.""" + assert bot._decision("yes or no, I am not sure", OPTS) is None + assert bot._decision("I can't say yes or no until Monday.", OPTS) is None + + +def test_line_order_still_beats_position(bot): + text = "No, sorry I can't.\nMaybe ask @alice - yes, she knows this genus." + assert bot._decision(text, OPTS) == "no" + + +def test_yaml_path_is_case_insensitive(bot): + assert bot._yaml_path_for("a/b/X_EAR.PDF") == "a/b/X_EAR.yaml" + assert bot._yaml_path_for("a/b/X_EAR.pdf") == "a/b/X_EAR.yaml" + assert bot._yaml_path_for("a/v1.pdf.d/X.pdf") == "a/v1.pdf.d/X.yaml" + + +def test_passerby_verdict_does_not_freeze_a_pr_with_someone_appointed(bot): + """The half of the passer-by case the previous suite left uncovered. + + A stranger's CHANGES_REQUESTED used to make this True, so the deadline + never fired, no successor was asked, and the appointed reviewer's + eventual "Yes" was discarded. + """ + pr = FakePR( + reviews=[FakeReview("passerby", "CHANGES_REQUESTED")], comments=[asked("rev")] + ) + assert not bot._review_in_progress(pr) + + +def test_reviews_by_is_newest_first(bot): + """closed_pr takes candidates[0]; chronological order credited the oldest.""" + pr = FakePR( + reviews=[FakeReview("rev", "CHANGES_REQUESTED"), FakeReview("rev", "APPROVED")], + comments=[asked("rev")], + ) + assert bot._reviews_by(pr, {"rev"})[0].state == "APPROVED" + + +def test_hand_assigned_reviewer_is_thanked_after_an_earlier_ask(bot): + """Supervisor assigns C by hand after the bot's candidate A declined.""" + pr = FakePR( + reviews=[FakeReview("carol", "APPROVED")], + comments=[asked("alice")], + events=[FakeEvent("review_requested", "carol")], + ) + assert any("for the review" in c for c in run_approve(bot, pr, "carol")) diff --git a/ear_bot/tests/test_roster.py b/ear_bot/tests/test_roster.py new file mode 100644 index 00000000..c0dff284 --- /dev/null +++ b/ear_bot/tests/test_roster.py @@ -0,0 +1,207 @@ +"""Storage layer: conflict handling, and recording a merge atomically.""" + +import pytest + +from ear_bot.roster import EAR_REVIEWS_CSV, REVIEWERS_CSV, Roster, RosterError + +from .conftest import HEADER, ROWS + + +def test_two_writes_in_one_run_both_succeed(repo, roster): + """find_reviewer() updates the roster once per PR in a single sweep.""" + roster.apply(reviewers={"alice"}, busy=True) + roster.apply(reviewers={"bob"}, busy=True) + assert len(repo.writes) == 2 + assert repo.roster_rows()["bob"]["Working PRs"] == "1" + + +def test_conflict_preserves_the_other_runs_change(repo, roster): + """The whole point of the SHA guard: never silently undo a concurrent write. + + Another run credits carol while we are marking bob busy. Our retry must + keep carol's change and add ours on top, not replay our stale snapshot. + """ + concurrent = f"{HEADER}\n" + ROWS.replace( + "carol,Carol C,Genoscope,Y,2,2025-03-03,Y,0,1000", + "carol,Carol C,Genoscope,Y,3,2026-01-01,Y,0,1111", + ) + repo.commit_concurrently(REVIEWERS_CSV, concurrent) + + roster.apply(reviewers={"bob"}, busy=True) + + rows = repo.roster_rows() + assert rows["carol"]["Calling Score"] == "1111", "concurrent change was clobbered" + assert rows["carol"]["Total Reviews"] == "3", "concurrent change was clobbered" + assert rows["bob"]["Working PRs"] == "1", "our own change was lost" + + +def test_conflict_does_not_double_apply_our_change(repo, roster): + """Re-running the mutation on retry must not apply it twice.""" + repo.commit_concurrently(REVIEWERS_CSV, f"{HEADER}\n{ROWS}") + roster.apply(reviewers={"alice"}, busy=True) + # alice starts at 1; exactly one increment must land. + assert repo.roster_rows()["alice"]["Working PRs"] == "2" + + +def test_failed_write_leaves_no_uncommitted_counters(repo, roster): + """If every retry fails, the in-memory roster must not keep the edits.""" + def always_conflict(_repo): + _repo.files[REVIEWERS_CSV][1] = "moved-again" + + repo.before_write[REVIEWERS_CSV] = always_conflict + repo.commit_concurrently = lambda *a, **k: None + original = repo.update_file + + def always_reject(path, message, content, sha): + if path == REVIEWERS_CSV: + raise RuntimeError("409") + return original(path, message, content, sha) + + repo.update_file = always_reject + with pytest.raises(RuntimeError): + roster.apply(reviewers={"alice"}, busy=True) + assert roster.data, "roster was left empty" + assert next(r for r in roster.data if r["Github ID"] == "alice")[ + "Working PRs" + ] == "1", "uncommitted edit survived the failure" + + +def test_unknown_reviewer_raises_in_strict_mode(roster): + with pytest.raises(RosterError): + roster.apply(reviewers={"ghost"}, busy=False) + + +def test_release_path_skips_unknown_and_frees_the_rest(repo, roster): + """A departed reviewer must not block CLEAR for everyone else.""" + roster.apply(reviewers={"ghost", "alice"}, busy=False, strict=False) + assert len(repo.writes) == 1 + assert repo.roster_rows()["alice"]["Working PRs"] == "0" + + +def test_blank_reviewer_id_is_treated_as_missing(roster): + """get_user_info() returns "" for a deleted account.""" + assert roster.missing([""]) == {""} + with pytest.raises(RosterError): + roster.apply(reviewers={""}, busy=False) + + +ROW = ["https://github.com/o/r/pull/9"] + [""] * 13 + + +def test_record_review_writes_nothing_when_reviewer_unknown(repo, roster): + with pytest.raises(RosterError): + roster.record_review( + row_values=ROW, reviewers=["ghost"], institution="Sanger", + submitted_at="2026-01-01", + ) + assert repo.writes == [] + + +def test_record_review_is_idempotent(repo): + repo.files[EAR_REVIEWS_CSV][0] = f"PR URL,Species\n{ROW[0]},Foo\n" + roster = Roster(repo) + assert roster.record_review( + row_values=ROW, reviewers=["alice"], institution="Sanger", + submitted_at="2026-01-01", + ) is False + assert repo.writes == [] + + +def test_record_review_logs_before_updating_counters(repo, roster): + """Order matters: the log row is the key that makes a re-run safe.""" + assert roster.record_review( + row_values=ROW, reviewers=["alice"], institution="Sanger", + submitted_at="2026-01-01", + ) is True + assert [p for p, _ in repo.writes] == [EAR_REVIEWS_CSV, REVIEWERS_CSV] + alice = repo.roster_rows()["alice"] + assert alice["Working PRs"] == "0" + assert alice["Total Reviews"] == "4" + + +def test_review_log_append_survives_a_conflict(repo, roster): + """A concurrent append must be preserved, not overwritten.""" + repo.commit_concurrently( + EAR_REVIEWS_CSV, "PR URL,Species\nhttps://github.com/o/r/pull/8,Other\n" + ) + roster.record_review( + row_values=ROW, reviewers=["alice"], institution="Sanger", + submitted_at="2026-01-01", + ) + urls = [r[0] for r in repo.review_rows()[1:]] + assert "https://github.com/o/r/pull/8" in urls, "concurrent row was lost" + assert ROW[0] in urls, "our row was lost" + + +def test_timeout_penalty_is_applied(repo, roster): + roster.apply(reviewers={"alice"}, busy=False, fined_reviewers={"alice"}) + assert repo.roster_rows()["alice"]["Calling Score"] == "1001" + + +def test_real_roster_round_trips_byte_for_byte(): + """Guards against the bot reformatting the whole file on its first write.""" + from rev import get_EAR_reviewer as g + + src = open("rev/reviewers_list.csv").read() + assert g.format_csv(g.parse_csv(src), g.csv_fieldnames(src)).strip() == src.strip() + + +def test_exists_reports_missing_and_present(repo): + from ear_bot.roster import exists + + assert exists(repo, REVIEWERS_CSV) + assert not exists(repo, "Assembly_Reports/x/x_EAR.yaml") + + +def test_concurrent_log_hit_does_not_double_apply_counters(repo, roster): + """If another run logs the row first, it owns the counters too.""" + repo.commit_concurrently( + EAR_REVIEWS_CSV, f"PR URL,Species\n{ROW[0]},Foo\n" + ) + before = repo.roster_rows()["alice"] + assert roster.record_review( + row_values=ROW, reviewers=["alice"], institution="Sanger", + submitted_at="2026-01-01", + ) is False + after = repo.roster_rows()["alice"] + assert after["Total Reviews"] == before["Total Reviews"] + assert after["Working PRs"] == before["Working PRs"] + + +def test_timeout_penalty_applies_with_no_reviewers_to_release(repo, roster): + """A reviewer asked twice then accepting leaves `reviewers` empty.""" + roster.apply(reviewers=set(), busy=False, fined_reviewers={"alice"}) + assert repo.roster_rows()["alice"]["Calling Score"] == "1001" + + +def test_unknown_fined_reviewer_is_ignored(repo, roster): + roster.apply(reviewers=set(), busy=False, fined_reviewers={"ghost"}) + assert repo.writes == [] + + +def test_a_landed_write_is_not_applied_twice(repo, roster): + """The response can be lost after GitHub has already committed. + + Treating every exception as "did not land" re-ran the mutation on top of + our own committed change, double-counting the review. + """ + real = repo.update_file + + def commit_then_fail(path, message, content, sha): + real(path, message, content, sha) # the write lands + raise RuntimeError("502 Bad Gateway") # the response does not + + repo.update_file = commit_then_fail + roster.apply(reviewers={"alice"}, busy=False, submitted_at="2026-01-01") + repo.update_file = real + + alice = repo.roster_rows()["alice"] + assert alice["Total Reviews"] == "4", "review counted twice" + assert alice["Calling Score"] == "999", "score adjusted twice" + assert alice["Working PRs"] == "0" + + +def test_mixed_case_unknown_fined_reviewer_is_dropped(repo, roster): + """missing() returns original case; the subtraction must lower-case it.""" + roster.apply(reviewers=set(), busy=False, fined_reviewers={"Ghost"}) + assert repo.writes == [], "committed an unchanged roster" diff --git a/rev/get_EAR_reviewer.py b/rev/get_EAR_reviewer.py index ab0657ad..72bb3ba7 100644 --- a/rev/get_EAR_reviewer.py +++ b/rev/get_EAR_reviewer.py @@ -5,6 +5,8 @@ import requests import random +import csv +import io from datetime import datetime import argparse @@ -24,10 +26,37 @@ def download_csv(url): return None def parse_csv(csv_str): - lines = csv_str.strip().split('\n') - headers = lines[0].split(',') - data = [dict(zip(headers, line.split(','))) for line in lines[1:]] - return data + # csv.DictReader rather than str.split(',') so that a quoted field + # containing a comma, for example "Aury, Jean-Marc", does not shift every + # following column. + # + # DictReader fills columns a short row does not reach with None, whereas + # the old dict(zip(...)) left the key out entirely so that .get(key, + # default) fell back to the default. Dropping the Nones keeps that + # behaviour, so one truncated hand-edit degrades instead of crashing every + # int() cast downstream. + reader = csv.DictReader(io.StringIO(csv_str.strip())) + return [ + {key: value for key, value in row.items() if value is not None} + for row in reader + ] + +def csv_fieldnames(csv_str): + """Column names in file order, parsed the same way as the rows.""" + return csv.DictReader(io.StringIO(csv_str.strip())).fieldnames or [] + +def format_csv(data, fieldnames=None): + """Render roster rows back to CSV, quoting anything that needs it.""" + if not data: + return '' + if fieldnames is None: + fieldnames = list(data[0].keys()) + buffer = io.StringIO() + writer = csv.DictWriter(buffer, fieldnames=fieldnames, lineterminator='\n') + writer.writeheader() + for row in data: + writer.writerow({key: row.get(key, '') for key in fieldnames}) + return buffer.getvalue() def normalize_institution(institution): institution = institution.lower() @@ -102,6 +131,9 @@ def select_best_reviewer(data, calling_institution, use_bge): top_score = eligible_candidates[0]['Adjusted Score'] if eligible_candidates else None top_candidates = [c for c in eligible_candidates if c['Adjusted Score'] == top_score] if top_score is not None else [] + if not top_candidates: + return eligible_candidates, [], "no eligible candidates" + if len(top_candidates) == 1: return eligible_candidates, top_candidates, "highest adjusted calling score in this particular selection"