Skip to content

feat(seidroid-review): acknowledge the trigger with a reaction #72

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bdchatham merged 1 commit into from
Sep 5, 2026
Merged

feat(seidroid-review): acknowledge the trigger with a reaction #72

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions .github/workflows/seidroid-review.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,6 +577,9 @@ jobs:
pull-requests: write # upsert the one sticky verdict comment
contents: read # read PR metadata
checks: write # publish the review check run
# React to the triggering comment. A reaction on a PR comment goes to the
# ISSUE comments endpoint, which pull-requests: write does not cover.
issues: write # acknowledge the trigger with a reaction

@github-actions github-actions Bot Sep 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] This is a reusable workflow, so the caller's token is the ceiling: a called job may only downgrade the caller's permissions, never elevate them. Where the calling job enumerates its permissions: (as every workflow in this repo does) without issues: write — or where the org restricts the default GITHUB_TOKEN — GitHub rejects the request rather than intersecting it, failing the whole review job before any step runs (The workflow is requesting 'issues: write', but is only allowed 'issues: none'). The step's continue-on-error cannot protect against that, because the failure is workflow validation, not the step.

The effect is the inverse of the change's purpose: a caller that bumps the pin without also adding the permission loses the entire review, not just the acknowledgement. Worth confirming sei-load#97 adds issues: write to the caller job and lands with the bump, and worth stating the new caller requirement in this file's header alongside the other caller-facing wiring.

# The credential lives ONLY here, at job level. It must never be re-declared
# as step-level env on a `uses:` step (composite/action steps do not receive
# step-level env at all) -- that is the exact defect that broke every real
Expand All @@ -594,6 +597,36 @@ jobs:
# happens here, the same way the machine-client check below does it.
HAS_REVIEWER_IDENTITY: ${{ secrets.SEIDROID_APP_ID != '' }}
steps:
# First, deliberately: the reaction is the only signal the trigger was
# seen, and everything after it -- toolchain, driver install, session
# start -- runs for minutes before anything else appears on the pull
# request. An acknowledgement that arrives after the verdict is not one.
#
# Comment path only. An automatic pull_request review has no comment to
# react to, so the guard leaves comment_id empty and this is skipped.
#
# continue-on-error: an acknowledgement is a courtesy. Failing the review
# because a reaction did not post would trade the whole job for the
# signal that the job started.
- name: Acknowledge the trigger
if: ${{ needs.guard.outputs.comment_id != '' }}
continue-on-error: true
env:
GH_TOKEN: ${{ github.token }}

@github-actions github-actions Bot Sep 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] The reaction lands as github-actions[bot] rather than as seidroid, because the identity mint runs later in the job. In a repository that also runs ai-review.yml under that same default identity, the 👀 does not say which automation saw the trigger — a weaker signal than the sticky verdict that follows it.

Not a straightforward fix, and reasonable to leave as is: the App token is scoped to review_repo_name || github.event.repository.name, so on a cross-repository trigger it is scoped to the reviewed repo and could not react to a comment living here. github.token is the only credential that always covers the comment being reacted to. If it seems worth aligning the attribution for the same-repo path, that would mean moving the mint above this step and falling back to github.token; otherwise the current choice is the correct one and the tradeoff is worth a line in the step's comment.

REPO: ${{ github.repository }}
TRIGGER_ID: ${{ needs.guard.outputs.comment_id }}
run: |
set -euo pipefail
# Reactions are idempotent per (user, content): re-running a review on
# the same comment returns the existing reaction rather than adding a
# second one, so a retry needs no cleanup.
if gh api -X POST "repos/$REPO/issues/comments/$TRIGGER_ID/reactions" \
-f content=eyes >/dev/null 2>&1; then

@github-actions github-actions Bot Sep 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] 2>/dev/null discards the API error at exactly the moment it is worth having. This change exists so a human can tell "seen" from "dropped" without opening the Actions tab; when the reaction itself fails, the warning says only that it failed, and the log no longer distinguishes a 403 (the caller did not grant issues: write) from a 404 (wrong comment id) from a rate limit — so the diagnosis moves back into the workflow log the change set out to avoid, minus the one line that would have explained it.

Capturing stderr and folding it into the warning keeps that, e.g.:

if err=$(gh api -X POST "repos/$REPO/issues/comments/$TRIGGER_ID/reactions" \
    -f content=eyes 2>&1 >/dev/null); then
  echo "acknowledged comment $TRIGGER_ID"
else
  echo "::warning::could not react to comment $TRIGGER_ID; the review continues: $(printf '%s' "$err" | tr '\n' ' ')"
fi

The tr matters: an annotation is single-line, so a multi-line gh error would otherwise truncate at the first newline.

echo "acknowledged comment $TRIGGER_ID"
else
echo "::warning::could not react to comment $TRIGGER_ID; the review continues"
fi

- name: Set up Go
uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0
with:
Expand Down
Loading