Rework EAR bot review/merge handling and extract roster storage - #407
Draft
arash77 wants to merge 4 commits into
Draft
Rework EAR bot review/merge handling and extract roster storage#407arash77 wants to merge 4 commits into
arash77 wants to merge 4 commits into
Conversation
find_reviewer() called _check_pr_activity() before checking the project
label, so it ran against every open PR in the repo. The bot has been
adding DELAYED and STALLED labels and posting weekly ping comments on
unrelated PRs, including the Dependabot ones. The project label check
now happens first. EAR PRs still get their activity check regardless of
the other skip conditions, as before.
Add a permissions block to all six workflows. The bot authenticates
with GITHUB_APP_TOKEN, so the ambient GITHUB_TOKEN needs nothing. It is
set to {} everywhere except 5_ear_bot_approved_comment.yml, which needs
actions: read to list and download the artifact from the triggering run.
Add requests to ear_bot/requirements.txt. rev/get_EAR_reviewer.py
imports it directly and it was only resolving because PyGithub happens
to depend on it.
Add the usual Python caches to .gitignore.
A drive-by comment review no longer blocks the bot. get_reviews() also returns COMMENTED reviews, which any user can leave on a public repo and which cannot be deleted. Both find_reviewer() and comment() treated that as "already reviewed", so one stray comment review stopped a PR from ever getting a reviewer, with no error and no label. There is now a _has_binding_review() helper that only counts APPROVED and CHANGES_REQUESTED. The CLEAR command now stops after clearing. It had no exit, so control fell through to the supervisor confirmation branch, which re-added the ERROR! label that CLEAR had just removed and exited 1. The OK confirmation is parsed like the Yes/No reply. It used a bare substring test over the whole comment, so "Looks good to me", "took a look" and "broken" all counted as OK, as did quoting the bot's own message. Both paths now share a _first_reply_line() helper and use word boundaries. approve_reviewer() checks the review that fired the event instead of pr.get_reviews()[0], which is the oldest review on the PR. Any earlier review by somebody else suppressed the thank-you comment, which closed_pr() later relies on to identify the reviewer. It also no longer crashes on a PR with no assigned supervisor. closed_pr() no longer crashes on a PR closed before a supervisor was assigned, resolves the EAR PDF before writing either CSV so a missing PDF cannot leave the data half-updated, and keys other_participants by GitHub ID so the roster Full Name lookup matches. It was comparing a lower-cased ID against a set of display names, so it almost never fired and the column mixed real names, display names and bare logins. The timeout penalty now applies. It was an elif on a branch that always won first, so the "Calling Score + 1" documented in the README never ran and a reviewer could ignore every request with no effect on their ranking. select_best_reviewer() returns an empty candidate list instead of raising IndexError when nobody is eligible, which made the "No eligible candidates found." message unreachable. Supervisor lookup is case-insensitive. Roster IDs were compared raw against a lower-cased author, so a supervisor whose login has any uppercase could not confirm a PR. Slack posts escape &, < and > in names taken from the PR body, so a species name cannot contain <!channel>.
commit() now takes the blob SHA the content was based on and passes it
to update_file, so GitHub rejects the write if the file changed in the
meantime. It used to re-read the SHA at write time, which made every
write succeed and silently discard whatever another workflow run had
committed in between. The scheduled run and the per-PR runs use
different concurrency groups, so they overlap freely.
commit() also raises instead of catching and printing. A lost update
used to look exactly like a success in the job log.
Roster read and write both go through the csv module. Reading used
line.split(',') and writing used ",".join(row.values()), so one comma
in any field shifted every following column, and the header was rebuilt
from dict key order rather than the file. add_pr() builds its row with
csv.writer for the same reason: species, names and institutions all
come from the PR body. Verified that the current reviewers_list.csv
round-trips byte for byte.
update_reviewers_list() raises when asked to update an ID that is not
on the roster. It used to skip silently, print "Updated the reviewers
list for <id>" and commit an unchanged file.
The previous commits fixed bugs one at a time in a file that was already too big to reason about, and several of those fixes introduced new problems. This moves the storage layer out and gives the merge path a single place where it either records everything or records nothing. ear_bot/roster.py now owns both CSVs. ear_bot_reviewer.py drops from 1043 to 951 lines. commit() is split. replace() is for files only the bot writes, such as the generated YAML. update_if_unchanged() takes a required sha, so forgetting it is no longer possible; the old sha=None default silently re-read and reintroduced the very race the parameter existed to close. A rejected roster write is now re-read and re-applied instead of failing. Previously a single conflict left the cached sha stale, so every remaining PR in the same scheduled run also failed, each one getting an ERROR! label and a supervisor ping after its reviewer had already been asked. record_review() checks every precondition before the first write and skips a PR already present in the log, so a failure part-way can no longer leave a review credited with the roster untouched, and a re-run cannot append a duplicate row. Releasing reviewers no longer requires everyone to still be on the roster. CLEAR and the Yes path pass strict=False, so one person having left the consortium cannot block the cleanup for everybody else. Commands are matched against any line the author wrote rather than only the first, and "Okay" is accepted. Requiring OK on the first line rejected a supervisor who opened with a greeting, and the failure path stamps ERROR! and fails the workflow. Quoted text is still ignored. Review detection is author-aware. GitHub clears the pending review request when the appointed reviewer submits any review, including a comment-only one, so filtering on review state alone let the bot declare "Time is out!" and reassign a PR mid-review. DISMISSED now counts as a verdict, so a merged PR whose approval was dismissed by branch protection is still recorded. approve_reviewer falls back to the requested reviewers when the bot never posted an ask comment, so a manually assigned reviewer is still thanked. Inactivity tracking covers EAR-UPDATE and ERROR! PRs again, and the no-review branch of closed_pr no longer dies on a missing PDF.
arash77
changed the title
This was referenced Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Draft. Supersedes #405 and #406, which are closed in favour of this single PR. Sits on top of #404 (hygiene + workflow permissions), so once #404 merges this diff shrinks to just the bot-logic changes.
Why one PR
This began as three stacked PRs. Successive reviews found that each round of point fixes introduced about as many regressions as it solved, because the file was too large to reason about. Merging the stack in sequence would land those known-broken intermediate states on
main(a tautological approval guard, a cascading write-conflict, "Okay" being rejected). This PR is the final state only, reviewed as one change.Storage
ear_bot/roster.pynow owns both CSVs.ear_bot_reviewer.pydrops from 1043 to 951 lines.commit()is split.replace()is for files only the bot writes (the generated YAML).update_if_unchanged()takes a requiredsha, so forgetting it is no longer possible; the oldsha=Nonedefault silently re-read the SHA and reintroduced the lost-update race the parameter existed to close.A rejected roster write is re-read and re-applied rather than failing. Previously one conflict left the cached SHA stale, so every remaining PR in the same scheduled run also failed, each getting an ERROR! label and a supervisor ping after its reviewer had already been asked.
record_review()checks every precondition before the first write and skips a PR already in the log, so a part-way failure can no longer credit a review with the roster untouched, and a re-run cannot append a duplicate row.Behaviour
approve_reviewerfalls back to the requested reviewers when the bot never posted an ask comment, so a manually assigned reviewer is still thanked.closed_prno longer dies on a missing PDF.Testing
33 checks, all passing: a simulated write conflict, an unknown reviewer on both the strict and lenient paths, idempotent re-recording, the roster round-tripping byte for byte, and one case per behaviour change above.
Still draft: this is the fourth round of changes on code that prior reviews kept finding regressions in, so it wants a fresh review before it comes out of draft.