Skip to content

Fix comment handling, review detection and reviewer bookkeeping - #405

Closed
arash77 wants to merge 2 commits into
ERGA-consortium:mainfrom
arash77:fix/bot-correctness
Closed

Fix comment handling, review detection and reviewer bookkeeping#405
arash77 wants to merge 2 commits into
ERGA-consortium:mainfrom
arash77:fix/bot-correctness

Conversation

@arash77

@arash77 arash77 commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Builds on #404, so that change shows up here too. Review #404 first, then this one; the second commit is the only new part.

A drive-by comment review no longer blocks a PR. get_reviews() also returns COMMENTED reviews, which any user can leave on a public repo and which cannot be deleted afterwards. 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. New _has_binding_review helper counts only APPROVED and CHANGES_REQUESTED.

CLEAR now stops after clearing. It had no exit, so control fell through into the supervisor confirmation branch, which re-added the ERROR! label CLEAR had just removed, posted "Invalid confirmation!" and exited 1.

OK is parsed like Yes and No. It used a bare substring test over the whole comment, so "Looks good to me", "took a look" and "broken" all counted as OK, and so did quoting the bot's own message back at it. Both paths now share _first_reply_line and use word boundaries.

approve_reviewer checks the review that fired the event instead of get_reviews()[0], which is the oldest review on the PR. Any earlier review by somebody else suppressed the thank-you comment, and closed_pr relies on that comment to identify the reviewer. It also no longer crashes when no supervisor is assigned.

closed_pr fixes: no crash on a PR closed before a supervisor was assigned, the EAR PDF is resolved before either CSV is written so a missing PDF cannot leave the data half updated, and other_participants is keyed 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 ended up mixing real names, display names and bare logins.

The timeout penalty applies again. It was an elif on a branch that always won first, so the "Calling Score + 1" in the README never ran and a reviewer could ignore every request with no effect on their ranking.

select_best_reviewer returns an empty list instead of raising IndexError when nobody is eligible, which had 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. Five roster rows already have uppercase logins.

Slack posts escape &, < and > in names taken from the PR body, so a species name cannot contain <!channel>.

Each of these was checked against the current code first to confirm it reproduces, then against the fix. 12 checks, all failing before and passing after. The ruff E402 warnings in get_EAR_reviewer.py are pre-existing and untouched.

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>.
@arash77
arash77 force-pushed the fix/bot-correctness branch from b268a7d to f554a81 Compare August 11, 2026 15:44
@arash77
arash77 marked this pull request as draft August 11, 2026 16:33
@arash77
arash77 marked this pull request as ready for review August 11, 2026 16:43
@arash77
arash77 marked this pull request as draft August 11, 2026 16:43
@arash77

arash77 commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author

Superseded by #407, which contains this commit plus the reworked storage layer and the fixes for the regressions a later review found in it. Consolidating into a single PR so no broken intermediate state can land on main.

@arash77 arash77 closed this Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant