Add review checklists workflow by LittleHuba · Pull Request #899 · eclipse-score/communication

LittleHuba · 2026-08-11T12:36:55Z

Adds a workflow to require acknowledgement of checklists by reviewers.

Currently no checklist is enforced.

+
+from __future__ import annotations
+
+import os


+import pytest
+import yaml
+
+import helpers


+import pytest
+import yaml
+
+import helpers


+            candidate = r.Rlocation(f"_main/{config_relpath}")
+            if candidate and os.path.isfile(candidate):
+                return candidate
+    except (ImportError, Exception):


+        monkeypatch.setenv("GITHUB_REPOSITORY", "org/repo")
+        monkeypatch.setenv("PR_NUMBER", "42")
+        gh = MagicMock()
+        repo, pr = get_repo_and_pr(gh)


+        monkeypatch.setenv("GITHUB_REPOSITORY", "org/repo")
+        monkeypatch.setenv("PR_NUMBER", "42")
+        gh = MagicMock()
+        repo, pr = get_repo_and_pr(gh)


castler

We need to extend the CI design also to explain why and for what we have these workflows.

Due to the size, let's discuss f2f which of the findings we tackle, tackle now or in a follow up.

castler · 2026-08-12T06:41:55Z

    name = "copyright",
    srcs = [
        ".github",
+        "actions",


Why this change?

Fixed, code was moved and this was not updated.

castler · 2026-08-12T07:28:44Z

+
+concurrency:
+  group: >-
+    review-checklists-apply-${{ github.event.workflow_run.head_sha || github.event.workflow_run.id }}


I think we are missing here a "cancel-in-progress" option? Or do we say this is so short running, that it does not make sense?

Adding the cancel-in-progress does not hurt. So I added it. But in general this is quite fast (given GitHub schedules the runs fast).

castler · 2026-08-12T08:39:24Z

+        GITHUB_TOKEN: ${{ inputs.github-token }}
+        GITHUB_REPOSITORY: ${{ github.repository }}
+        PR_NUMBER: ${{ inputs.pr-number }}
+      run: python3 "$GITHUB_ACTION_PATH/scripts/post_checklists.py" --config-path "${{ inputs.config-path }}"


We should make a comment here that we we do not on purpose start this with Bazel, and what was our reasoning for it.

Added comment.

castler · 2026-08-12T08:41:59Z

@@ -0,0 +1,109 @@
+# *******************************************************************************


Wondering if we do not want to place it also under actions/00_infrastructure or similar?

Not quite sure about that. Separating the code over the repo is not perfect, since this file contains logic that is closely related. GitHub actually has no convention where actions must be placed. So keeping it here is perfectly fine.

castler · 2026-08-12T09:00:17Z

+   ruleset to require the commit status context **`review-checklists`**
+   (the one this action sets via `set_commit_status`). Requiring the
+   workflow job itself will not gate merges correctly.
+5. **Require at least one approving review** in branch protection. The


Are we sure this is possible with Otterdog? As per https://otterdog.readthedocs.io/en/latest/reference/organization/repository/status-check/ I do not believe so?

Commit status checks are viable with any: prefix. I extended the documentation. Of course this is subject to trials.

castler · 2026-08-13T05:20:22Z

+        return is_in_queue
+
+    print("GraphQL merge-queue lookup returned no boolean state")
+    return False


Should we not throw here and rather catch that. Are we sure that IF we would be in the merge queue but the lookup fails for what ever reason (e.g. API error on the graphQL call) that FALSE would not bring us problems?

This branch is only used to provide the warning comment if a PR is modified after entering the merge queue. The worst happening here would be that the comment is wrongly emitted.
Still this omission would not modify the evidence.

castler · 2026-08-13T05:39:20Z

+
+    repo, pr = get_repo_and_pr(gh)
+
+    if is_pr_in_merge_queue(pr):


This I do not get, why do we not execute this in the if statement above?

if event_name == "merge_group":

then the PR needs to be in the merge_queue, no?

We want to add this warning comment when the PR is modified after it enters the merge queue. This means we react to events like pull_request_comment, pull_request_review.
Hence, we do not enter the first if condition at all but must still verify whether the PR is in the merge queue or not.

castler · 2026-08-13T05:44:51Z

+from typing import Any
+
+from helpers import (
+    _collect_acknowledgement_details,


I personally think its weird to "import" private functions.

castler · 2026-08-13T05:59:39Z

+        return "success", "No checklists applicable"
+
+    existing = find_existing_checklist_comments(pr)
+    relevant_ids = [cl["id"] for cl in relevant if cl["id"] in existing]


As per my current understanding, this would filter out only checklists where a comment has been made. Thus, if for a checklist no comment has been made (yet), we would skip it here.

Thus we would set success later, if for another checklist is succesful. Do we not rather have to "pend" on that all comments have been posted?

castler · 2026-08-13T06:05:37Z

+    for comment in pr.get_review_comments():
+        reply_to = getattr(comment, "in_reply_to_id", None)
+        if reply_to is None or reply_to not in cl_comment_ids:
+            continue
+
+        cid = cl_comment_ids[reply_to]  # type: ignore[index]
+        body = (comment.body or "").strip()
+        user = comment.user.login
+
+        if body.upper() == OK_KEYWORD:
+            acks[cid].add(user)
+
+    return acks


This functionality is duplicated.

See for example: def _find_ok_comments_for_checklist

I think we can extract it into an helper. (I am aware that we store different information, but I think we can make a super set of information returned?

The composite action was later moved from actions/review_checklists to tools/review-checklists, but the root BUILD file's copyright_checker srcs list still referenced the now-nonexistent "actions" directory (a no-op glob) and never gained a "tools" entry, so the new tools/review-checklists sources were not covered by the copyright check at all. Addresses review comment: #899 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Multiple non-merge_group events (e.g. several review comments posted in quick succession) can each trigger a completed 'Review Checklists (Trigger)' run for the same head SHA, spawning overlapping apply-stage runs in the same concurrency group. Let a newer run cancel an older, now-superseded one instead of leaving it queued/running: the job only recomputes idempotent PR state (comments/statuses) from scratch each time, so cancelling a stale run is safe and matches this repo's existing convention of setting cancel-in-progress explicitly. Addresses review comment: #899 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Addresses review comment: #899 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Otterdog's status-check reference distinguishes plain commit statuses (which need the 'any:' prefix) from GitHub Actions workflow-job or GitHub App statuses. The 'review-checklists' status this action sets via create_status is a plain commit status, so an Otterdog-managed branch protection rule/ruleset must reference it as 'any:review-checklists' — the bare name would not be recognized. Addresses review comment: #899 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

helpers.find_ok_replies (unused in production), helpers.collect_ acknowledgement_details, check_acknowledgements._collect_ok_ acknowledgements, and dismiss_and_invalidate._find_ok_comments_for_ checklist each independently re-implemented the same scan for OK-reply comments to a checklist finding. Extract a single shared helpers.find_ok_replies_for_checklists(pr, existing_comments, checklist_ids) that returns the 'superset' (full comment objects, in one pass over pr.get_review_comments()) and have each caller project it down to the narrower shape it needs. This also removes an O(n*k) re-scan in dismiss_and_invalidate.handle_synchronize (one scan per affected checklist) in favor of a single O(n) pass. Also renamed the now-shared collect_acknowledgement_details parameter 'relevant_ids' to 'checklist_ids' for clarity, since it isn't obvious from the name alone what the ids are relevant to. Addresses review comments: #899 (comment) #899 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

'_main' is the current Bzlmod-internal runfiles-directory name for the root module. Rlocation() accepts the module name directly and resolves it through the runfiles repo mapping to whichever directory the root module is actually materialized under. Use 'score_communication' (this repo's declared module name in MODULE.bazel) instead — this is the portable, correct API usage, and keeps working even if Bazel's internal main-repo directory naming convention changes, instead of depending on that implementation detail directly. Addresses review comment: #899 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

get_reviews() returns every review a user has ever submitted on the PR (in submission order), not just their latest state, so a user can approve and later request changes (or have an approval dismissed) in a subsequent review. The approvers.discard() call is what removes such a since-superseded approval recorded earlier in the same loop — it is not a no-op, even though 'approvers' starts as an empty set. Addresses review comment: #899 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Addresses review comment: #899 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Fail-closed to False on any lookup error is intentional here: the only callers use the result to decide whether to (re-)post an advisory merge-queue notice on the PR, so a false negative merely delays that notice — it never affects merge gating. The actual gating decision for merge_group events is made independently in check_acknowledgements.py, which resolves and validates the underlying PR itself and fails the commit status (rather than defaulting to success) if that resolution or validation fails. Addresses review comment: #899 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

_validate_checklist_evidence (used for merge_group evidence validation) and main()'s live-PR flow both derived 'relevant_ids' as only the relevant checklists that already have a posted finding comment, and then treated an empty relevant_ids as the only 'not all posted yet' case. If some but not all relevant checklists had been posted (e.g. a partial post_checklists run, or a checklist added to the config after others were already posted), the unposted ones were silently dropped from consideration entirely, and the status could go to 'success' once the posted subset was fully acknowledged — without a checklist that was never even posted ever being acknowledged. Fixed to require every relevant checklist to have a posted comment before proceeding past 'pending'. Addresses review comment: #899 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

… handling Explain that this is_pr_in_merge_queue() check in main()'s regular (non-merge_group) flow is intentionally distinct from the merge_group handling above it: it covers events fired for a PR that is still enqueued but is not itself a merge_group event (e.g. a review comment posted while the PR is queued), so the advisory merge-queue notice can be (re-)posted for it. Addresses review comment: #899 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Rename 'cl' -> 'checklist' and 'cid' -> 'checklist_id' (and 'cl_copy' -> 'checklist_copy', 'comment_id_to_cid' -> 'comment_id_to_checklist_id') throughout helpers.py, check_acknowledgements.py, dismiss_and_invalidate.py, post_checklists.py, and helpers_test.py. These abbreviations made call sites and comprehensions harder to scan at a glance, especially where 'cl' (checklist) could be misread as a different kind of list/class abbreviation. Wrapped the handful of lines that exceeded 88 columns after the rename. Addresses review comments (previously deferred by the reviewer to a future PR): #899 (comment) #899 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Following up on discussion_r3772590463 ('relevant_ids is a bad naming ... what is it relevant for?'): the same ambiguity existed one level up in the variables holding the matched checklist dicts themselves. 'relevant' and 'affected' read as bare adjectives with no object, which invited exactly this kind of question at every read site. Rename to 'relevant_checklists' (helpers.match_checklists/build_evidence_block, check_acknowledgements.py, post_checklists.py) and 'affected_checklists' (dismiss_and_invalidate.py) to make explicit what they are relevant/affected checklists of. Wrapped two comprehensions that exceeded 88 columns after the rename. No behavior change; verified with bazel test //tools/review-checklists/... Addresses review comment: #899 (comment) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>