Skip to content

feat(seidroid-review): publish the review check under both names #92

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 4 commits into from
Sep 6, 2026
+97 −27
Merged

feat(seidroid-review): publish the review check under both names #92

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
063d596
feat(seidroid-review): publish the review check under both names
bdchatham Sep 6, 2026
819a355
docs(seidroid-review): say only what the check-run listing shows
bdchatham Sep 6, 2026
bf483be
fix(seidroid-review): supersede both names on the failure path, and l…
bdchatham Sep 6, 2026
c4a006f
docs(seidroid-review): say what the AI Review switch does not do
bdchatham Sep 6, 2026
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
124 changes: 97 additions & 27 deletions .github/workflows/seidroid-review.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ run-name: UCI / seidroid review / ${{ (github.event.issue.number || github.event
# through the `driver-version` default. A thin caller in the reviewed repo wires the
# triggers and calls this with `uses:`. Flow: comment `@seidroid review` on a pull
# request -> guard gate -> install and run the driver over one managed omnigent
# session -> post the verdict as a new comment, the findings it can place, and a
# check run.
# session -> post the verdict as a new comment, the findings it can place, and its
# check runs.
#
# TWO PATHS, one review. An AUTOMATIC review runs on `pull_request` when the caller
# wires that trigger and passes `mode: review`. A MANUAL one runs when a person
Expand Down Expand Up @@ -122,6 +122,23 @@ on:
required: false
type: boolean
default: false
publish-ai-review-check:
description: >-
Publish the review's check run a second time, under the name `AI Review`.
On by default. A branch-protection rule matches a check by its name, the
rules on a calling repository are org-level, and no token here reads them.
A repository that retires `ai-review.yml` under a rule that names
`AI Review` waits forever on a check nothing publishes. That rule reads as
pending, not failed.

Set this false where the repository still runs `ai-review.yml` AND a rule

@github-actions github-actions Bot Sep 6, 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] The guidance here is narrower than the actual collision. It says to set this false only where the repository runs ai-review.yml AND a rule names AI Review, but the harm the next sentence describes — "GitHub lists only the check run that completed last, and this copy can hide the incumbent's verdict" — happens whenever both tools publish the name under this App, rule or no rule. With no rule, nothing blocks the merge, but the checks list still shows seidroid's conclusion in place of ai-review.yml's; a green copy landing after a red incumbent silently replaces the visible signal a human reads.

The step comment at the publish site ("A repository that also runs ai-review.yml under this App gets two checks called AI Review on the commit") states it correctly without the rule qualifier. Suggest dropping the AND a rule names \AI Review`` condition here so the input's own documentation matches it.

names `AI Review`. Both tools then publish that name, GitHub lists only the
check run that completed last, and this copy can hide the incumbent's
verdict. This switch does not remove that race. It gives the caller the
choice to accept it or to stand the copy down.
required: false
type: boolean
default: true

@github-actions github-actions Bot Sep 6, 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] Defaulting the copy to true swaps a fail-safe failure mode for an unsafe one.

The two error states are not symmetric:

  • Copy off, rule requires AI Review, nothing publishes it → check stays pending → merge is blocked. Noisy, but safe.
  • Copy on, repo still runs ai-review.yml, seidroid completes last → seidroid's conclusion is the one filter=latest returns. If ai-review.yml concluded failure and seidroid concludes success, the required check reads green and the merge that the incumbent blocked goes through.

The step comment and the input description both name the race, but the default picks the side where the failure mode is a bad merge rather than a stuck one. Since a caller that has already retired ai-review.yml is the only one that needs the copy, and that caller is by definition making a deliberate migration change, default: false with an explicit opt-in gets the same coverage without ever letting one tool's verdict mask the other's. If the owner has decided on true (PLT-1152), consider at least scoping it to a documented removal date so no repo silently carries the race indefinitely.

skip-review-label:
description: >-
A label on the reviewed pull request that stops the review. Empty
Expand Down Expand Up @@ -838,7 +855,7 @@ jobs:
permissions:
pull-requests: write # post the verdict comment and the review position
contents: read # read PR metadata
checks: write # publish the review check run
checks: write # publish the review's check runs
# 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
Expand Down Expand Up @@ -1347,10 +1364,9 @@ jobs:
echo "findings: $on_line on a line, $on_file on a file, $unplaced in the summary"

- name: Publish the review check run
# The half of a review a reader sees without opening it. Named review
# rather than "AI Review": both systems run during the transition and both
# post as seidroid[bot], so two checks under one name would be unreadable,
# where a green AI Review beside a red review is not.
# The half of a review a reader sees without opening it. `review` is the check
# of record. The same verdict goes out under `AI Review` too, for the reason
# the second publish below states.
#
# Whichever check the driver wrote, decided or not. A run that reached no
# verdict writes one under the title `no verdict`, naming why, and publishing
Expand Down Expand Up @@ -1380,6 +1396,7 @@ jobs:
PR: ${{ needs.guard.outputs.pr_number }}
REVIEWED_SHA: ${{ steps.head.outputs.sha }}
CHECK: ${{ steps.drive.outputs.check_path }}
AI_REVIEW_COPY: ${{ inputs.publish-ai-review-check }}
# The one field this step will not read out of the artifact. See below.
VERDICT_PRODUCED: ${{ steps.drive.outputs.verdict_produced }}
run: |
Expand All @@ -1402,6 +1419,12 @@ jobs:
echo "::error::the check file on $REPO#$PR carries no conclusion, so there is nothing to publish as the merge gate"
exit 1
fi
# The same default again. A jq that fails leaves the title empty, and the `//`
# above only covers a field that is absent. The summary needs no such line,
# because its default is the empty string either way.
title="$(jq -r '.title // "review"' "$CHECK" || true)"
title="${title:-review}"
summary="$(jq -r '.summary // ""' "$CHECK" || true)"
# Against the commit the review read, recorded before it started and used
# as recorded. A check on any other commit attaches this verdict to code the
# review never saw, so the head is not read again here.
Expand All @@ -1414,14 +1437,44 @@ jobs:
echo "::error::the reviewed commit was not recorded on $REPO#$PR, so the review check run cannot be published; the verdict comment is the only record of this review"
exit 1
fi
gh api -X POST "repos/$REPO/check-runs" \
-f name=review \
-f head_sha="$head_sha" \
-f status=completed \
-f conclusion="$conclusion" \
-f output[title]="$(jq -r '.title // "review"' "$CHECK")" \
-f output[summary]="$(jq -r '.summary // ""' "$CHECK")" >/dev/null
echo "published review check: $conclusion — $(jq -r '.title // "review"' "$CHECK")"
publish() {
gh api -X POST "repos/$REPO/check-runs" \
-f name="$1" \
-f head_sha="$head_sha" \
-f status=completed \
-f conclusion="$conclusion" \
-f output[title]="$title" \
-f output[summary]="$summary" >/dev/null
}
# The check of record, and the only publish here that can fail the step. A
# failure exits it under set -e, and continue-on-error above decides what
# that costs the job.
publish review
names="check run review"
# The same verdict under the name ai-review.yml publishes. A branch-protection
# rule matches a check by its name, so only a check called `AI Review`
# satisfies a rule that requires `AI Review`. The rules on the calling
# repositories are org-level, and no token here can read them. This step
# therefore publishes both names, and a rule that requires either one passes.
# Delete this call once someone who can read those rules confirms that none
# names `AI Review`.
#
# Best-effort, and that is the whole difference from the publish above. A
# failure here warns and the step still succeeds, so the copy never decides
# whether the review reached the pull request.
#
# A repository that also runs ai-review.yml under this App gets two checks
# called `AI Review` on the commit. GitHub keeps both and lists one: the one
# that completed last. The two publishes therefore race for the name, and
# `publish-ai-review-check` is how such a repository stands this copy down.
if [ "${AI_REVIEW_COPY:-}" = "true" ]; then
if publish "AI Review"; then
names="check runs review and AI Review"
else
echo "::warning::the AI Review copy of the review check could not be published on $REPO#$PR; review carries the verdict"

@github-actions github-actions Bot Sep 6, 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] A failed AI Review publish reintroduces exactly the state this input exists to prevent, and does it silently.

If publish "AI Review" fails (transient 5xx, secondary rate limit, token scope), this warns and the step succeeds. A caller that retired ai-review.yml under a rule naming AI Review is then back to the pending-forever case from the input description — a green job, a warning annotation nobody reads, and a merge blocked on a check that will never arrive.

Keeping the copy from failing the review is right; making it indistinguishable from success is not. ::error:: instead of ::warning:: would at least surface it as an annotation on the run without changing the step's exit, and it costs nothing given continue-on-error already governs this step.

fi
fi
echo "published $names: $conclusion — $title"
Comment thread
cursor[bot] marked this conversation as resolved.

- name: State the review's position on the pull request
# The check run is the gate a merge reads; this is the one a person reads,
Expand Down Expand Up @@ -1829,6 +1882,9 @@ jobs:
# the review started; absent only when that read failed, and then the
# annotation is the only record.
REVIEWED_SHA: ${{ steps.head.outputs.sha }}
# Read for the failure check below, which publishes under the same names the
# publish step does. One switch drives both, so the set of names matches.
AI_REVIEW_COPY: ${{ inputs.publish-ai-review-check }}
# The notice an earlier no-verdict run left, which this verdict supersedes.
# Its own name and its own value: a notice that reports no verdict has to be
# addressable apart from the verdict itself.
Expand Down Expand Up @@ -2036,20 +2092,34 @@ jobs:
echo "--- verdict, unposted ---"
printf '%s\n' "$body"
echo "--- end verdict ---"
# Under the same name as the check published above, so it supersedes that
# conclusion on this commit rather than sitting beside it, and so a later run
# that does post clears it. Best-effort: whatever stopped the comment can stop
# this too, and then the annotation stands alone.
# Under every name the publish step above writes. This conclusion then
# supersedes each of them on this commit rather than sitting beside one, and a
# later run that does post clears them. A copy left carrying the earlier
# conclusion is a rule reading green on a verdict that never arrived.
#
# `review` goes first and is the check of record. The `AI Review` copy follows
# it, under the switch that step reads, so the two publish the same set of
# names on both paths.
#
# Best-effort, both of them: whatever stopped the comment can stop this too,
# and then the annotation stands alone.
if [ -n "${REVIEWED_SHA:-}" ]; then
gh api -X POST "repos/$REPO/check-runs" \
-f name=review \
-f head_sha="$REVIEWED_SHA" \
-f status=completed \
-f conclusion=failure \
-f output[title]="review produced but not published" \
-f output[summary]="The review ran and reached a verdict. Posting it to this pull request failed, so the verdict is not here. Read it in the workflow run: $GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" \
>/dev/null \
fail_check() {
gh api -X POST "repos/$REPO/check-runs" \
-f name="$1" \
-f head_sha="$REVIEWED_SHA" \
-f status=completed \
-f conclusion=failure \
-f output[title]="review produced but not published" \
-f output[summary]="The review ran and reached a verdict. Posting it to this pull request failed, so the verdict is not here. Read it in the workflow run: $GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" \
>/dev/null
}
fail_check review \
|| echo "::warning::the failure check run could not be posted either; the annotation on this run is the only record"
if [ "${AI_REVIEW_COPY:-}" = "true" ]; then
fail_check "AI Review" \
|| echo "::warning::the AI Review copy of the failure check run could not be posted on $REPO#$PR; it still carries this review's earlier conclusion"
fi
else
echo "::warning::no reviewed commit was recorded, so there is no check run to fail; the annotation on this run is the only record"
fi
Expand Down
Loading