Rework EAR bot review/merge handling and extract roster storage - #407
Draft
arash77 wants to merge 9 commits into
Draft
Rework EAR bot review/merge handling and extract roster storage#407arash77 wants to merge 9 commits into
arash77 wants to merge 9 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
There was a problem hiding this comment.
Pull request overview
Reworks EAR bot reviewer assignment, merge bookkeeping, CSV storage, and workflow permissions.
Changes:
- Extracts roster and review-log persistence into
ear_bot/roster.py. - Improves command parsing, review detection, cleanup, and merge handling.
- Adds CSV-safe formatting and least-privilege workflow permissions.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
rev/get_EAR_reviewer.py |
Adds robust CSV parsing and empty-candidate handling. |
ear_bot/roster.py |
Introduces roster and review-log storage. |
ear_bot/requirements.txt |
Adds the direct requests dependency. |
ear_bot/ear_bot_reviewer.py |
Reworks reviewer and merge behavior. |
.gitignore |
Ignores Python caches and environments. |
.github/workflows/1_ear_bot_pr.yml |
Restricts token permissions. |
.github/workflows/2+4_ear_bot_comment.yml |
Restricts token permissions. |
.github/workflows/3_ear_bot_reviewer.yml |
Restricts token permissions. |
.github/workflows/5_ear_bot_approved.yml |
Restricts token permissions. |
.github/workflows/5_ear_bot_approved_comment.yml |
Grants artifact read permission only. |
.github/workflows/6_ear_bot_merge.yml |
Restricts token permissions. |
Suppressed comments (3)
ear_bot/roster.py:237
- This append can fail after
apply()has already committed the roster. Different PR workflow groups can fetch the same review-log SHA, so one append is rejected; rerunning then applies the roster update again before retrying the still-missing log row, double-adjusting counters. Make the cross-file operation idempotently recoverable instead of leaving the second write as a one-shot commit.
update_if_unchanged(
self.repo, EAR_REVIEWS_CSV, "Add new EAR review", text + buffer.getvalue(), sha
)
ear_bot/ear_bot_reviewer.py:761
- Author-aware detection is bypassed for verdict reviews: any passer-by's APPROVED, CHANGES_REQUESTED, or DISMISSED review returns
Truebefore the appointed users are checked. Since this code also recognizes that public users can approve, such a review can prevent the bot from assigning the roster reviewer indefinitely. Verdicts must also be associated with the appointed/requested reviewer.
if self._binding_reviews(pr):
return True
ear_bot/ear_bot_reviewer.py:530
- When no thank-you comment exists, this fallback credits the newest verdict without checking who was appointed. For example, after the appointed reviewer requests changes, a later passer-by approval becomes
binding_reviews[0]; the merge then credits the wrong person or rejects recording because that person is absent from the roster, leaving the actual reviewer busy. Resolve the authorized reviewer from the ask/request history before selecting their verdict.
binding_reviews[0],
馃挕 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A review of the previous commit found that its conflict retry did the opposite of what its own docstring claimed. On a rejected write it snapshotted every row, re-read the file, and then copied the whole stale snapshot back over the fresh rows, discarding whatever the concurrent run had just committed. That is the exact lost update the SHA guard exists to prevent, reintroduced by the code meant to handle it. _write() now takes the change as a callable and re-runs it against the re-read rows, so the other run's commit survives and ours is layered on top. It also reloads after a final failure, so a caller is never left holding counters that were never written. record_review() now writes the log row before the roster counters, and retries the log append. The log row is the idempotency key: if the roster write then fails, a re-run sees the row and stops, leaving a reviewer uncredited, which shows up as a stuck Working PRs count. The previous order failed the other way, applying the counters with no row to record that it had happened, so a re-run applied them again and corrupted scores silently. A blank reviewer ID now counts as missing. get_user_info() returns "" for a deleted GitHub account, and treating that as known let a merge be half-recorded: a log row with empty reviewer fields, and a roster update that quietly did nothing. Yes/No is decided by whichever the author wrote first, not by testing the whole comment for "yes" before "no". "No, sorry. Ask Alice, yes she knows this genus" was being read as an acceptance, appointing someone who had just declined. Review detection now looks at the current reviewer only: the most recent person asked, plus anyone GitHub still lists as requested. The previous version consulted the entire ask history, so a reviewer who had already timed out could block the PR forever, and it missed hand-assigned reviewers entirely. Reviews by a deleted account no longer crash it, and a merged PR is recorded when the appointed reviewer left only a comment review rather than being skipped and leaving them marked busy. approve_reviewer's fallback for hand-assigned PRs now reads the review-request events. pr.requested_reviewers cannot work there, because GitHub drops a reviewer from that list the moment they submit a review, so by the time --approve runs it never contains them. Adds ear_bot/tests: 34 tests over a fake GitHub API that enforces the real update_file SHA contract, so the concurrency behaviour is actually exercised. 14 of them fail against the previous commit.
Two gaps left over from the previous commit. The recordable-review set still fell back to any verdict review, so on a public repo a stranger's approval could be written into EAR_reviews.csv as the review of the PR. It is now limited to people actually on the hook: the appointed reviewer, anyone GitHub still lists as requested, or whoever the bot already thanked. A comment-only review from that person still counts, since clicking Comment instead of Approve is a mis-click, not a reason to withhold the credit. The problem path no longer releases the reviewer. WF6 fires again if a PR is reopened and re-closed, so an automatic release decremented Working PRs once per run. It now retains the count and asks for CLEAR, which is what the bot already does for a PR closed unmerged and is issued by a human once.
Both raised by Copilot on the PR. closed_pr() returned as soon as the review row already existed, so if an earlier run recorded both CSVs and then died before generating the YAML, every later run stopped at the same place and the YAML was never produced. It now generates the YAML when it is missing regardless of the dedup hit. The Slack post is deliberately not retried: it has no idempotency key, and re-announcing an assembly to the whole consortium is worse than a human reposting one that was missed. find_reviewer()'s docstring still described the old behaviour, saying PRs without a project label are skipped entirely. Since EAR-UPDATE and ERROR! PRs were given back their activity processing, that was wrong in exactly the way that invites someone to "simplify" the gate later and silently drop the weekly ping again.
All raised by a review of the previous commits, and all introduced by them. _append_review() returned the same None whether it wrote the row or found it already present, so record_review() applied the roster counters either way. When another run won the race and logged the review first, this run credited the same review a second time and, because it still reported success, closed_pr() re-posted the Slack announcement. It now returns whether it wrote, and record_review() leaves the counters to whoever logged it. record_review()'s docstring still described the old roster-first order, contradicting both the code and its own inline comment. The log-first order is load-bearing for idempotency, so a maintainer trusting the docstring would have reintroduced the double-apply. The module docstring had the same error. apply() returned early whenever the reviewer set was empty, dropping any timeout penalty travelling with it. A reviewer asked, timed out, re-asked and then accepting leaves that set empty, so the documented "Calling Score + 1" never landed. The Yes/No decision now takes the leftmost match on the first line that answers, rather than treating any line with both words as ambiguous. "Yes, no problem." was being rejected with "Invalid confirmation!" and a re-ask. Line order still wins over position, so a refusal that mentions "yes" later is still a refusal. The YAML path is derived with splitext through one helper. The finder that picks the PDF matches case-insensitively, so a file committed as *.PDF produced a YAML path identical to the PDF: on one branch that silently skipped generation, on the other it read the binary PDF as text and would have overwritten it. A merged PR where nothing the appointed reviewer wrote is recordable now warns the supervisor and applies ERROR!, matching the two sibling paths. It previously just regenerated the YAML, leaving the reviewer permanently busy with nobody told. 43 tests; 4 of the new ones fail against the previous commit.
Three reviews of the previous commits converged on these. All were mine. A landed write is no longer applied twice. Roster._write caught every exception as "the write did not land", but only a stale SHA proves that. If GitHub committed the change and the response was then lost, _load() re-read our own committed change, the mutation ran again on top of it, and the retry succeeded -- silently double-counting a review. It now compares the freshly read file against the payload it just sent and returns if they match. The residual race, where our write lands and another run commits on top before we re-read, is documented rather than papered over: closing it needs a per-operation idempotency key that the roster CSV has no column for. A passer-by can no longer freeze a PR. _review_in_progress accepted a verdict from anybody as long as somebody was appointed, so a stranger's CHANGES_REQUESTED on this public repo stopped the deadline firing, no successor was ever asked, and the appointed reviewer's eventual "Yes" was discarded without a word on the PR. It is now strictly author-based. The comment above it had been claiming this all along. test_yes_and_no_on_one_line_takes_the_leftmost asserted that "No problem - yes I'll take it" means no. It does not, and neither do "Sure, no problem" or "No worries, I can review it", all of which the bot was reading as declines and reassigning the assembly. The test codified the bug as the specification, which is worse than the bug. The NO pattern now excludes the "no problem/worries/..." idioms, which is the real distinction, and the ambiguity guard is restored: taking the leftmost match had turned "I can't say yes or no until Monday" into an acceptance. Smaller, each raised by two reviewers: _reviews_by now matches _binding_reviews in returning newest first, since closed_pr takes candidates[0] and was crediting the oldest verdict; the fined set lower-cases before subtracting, so a mixed-case unknown ID no longer survives to trigger an empty commit; _first_reply_line and _has_binding_review are deleted as dead code, the former encoding superseded semantics. 5_ear_bot_approved.yml gets contents: read. Its checkout is the only one with no token: input, and two reviews disagreed on whether a public repo still grants that by default. Making it explicit costs nothing. The comment on the activity gate claimed the old code gated on valid_projects. It did not -- it ran on every open PR, which is why the bot was pinging Dependabot. Corrected. 49 tests. The two data-integrity ones fail against the previous commit.
Contributor
|
Ping @arash77, |
2 similar comments
Contributor
|
Ping @arash77, |
Contributor
|
Ping @arash77, |
Contributor
|
Ping @arash77, |
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
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.