Skip to content

fix(seidroid-review): fail soft when the step cannot post the verdict #70

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

Closed
bdchatham wants to merge 1 commit into from
+100 −6
Closed

fix(seidroid-review): fail soft when the step cannot post the verdict #70

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
106 changes: 100 additions & 6 deletions .github/workflows/seidroid-review.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1080,7 +1080,13 @@ jobs:
# Keying on verdict_produced, not the exit code, so a teardown-only
# failure still posts a valid verdict and a no-verdict run never
# upserts a placeholder.
#
# continue-on-error, like every other publish step. This one is the last thing
# standing between a finished review and the reader, and failing the job here
# throws that review away rather than saving it. What it costs is the signal,
# so the run block states the failure itself; see there.
if: ${{ inputs.mode == 'review' && (!cancelled() && steps.drive.outputs.verdict_produced == 'true') }}
continue-on-error: true

@github-actions github-actions Bot Sep 4, 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] continue-on-error covers the whole step, but the replacement signal (::error:: + failure check run) only runs on the gh api failure path below. Any other set -e abort earlier in the script — cat "$VERDICT", wc, head, the $(( )) arithmetic — now exits the step before line 1188 is reached, and the flag turns that into a green job with no annotation and no check run. That is precisely the "silently absent review" the description argues is worse than a red job; before this change those failures were at least loud.

An ERR/EXIT trap installed right after set -euo pipefail would close the gap, e.g. trap '[ "$posted" = true ] || echo "::error::the review reached a verdict but the publish step failed; see this log"' EXIT (with posted=false initialised at the top).

shell: bash
env:
GH_TOKEN: ${{ steps.identity.outputs.token || github.token }}
Expand All @@ -1095,20 +1101,108 @@ jobs:
# Findings the step above could place nowhere on the diff. Empty when it
# placed them all, and when it was skipped for a cross-repository review.
NOTE: ${{ steps.place.outputs.note_path }}
# The commit the review read, for the failure check below. Recorded before
# the review started; absent only when that read failed, and then the
# annotation is the only record.
REVIEWED_SHA: ${{ steps.head.outputs.sha }}
# GitHub rejects an issue comment over 65,536 characters. The driver bounds
# the verdict it writes and clips its own text to fit (review.MaxBodyBytes,
# 60,000). Nothing bounds $NOTE: it carries Finding.Detail, raw model prose
# with no length constraint on it. Unbounded, a long tail of unplaced
# findings pushes the body past the cap and the upsert below is rejected --
# losing a whole review over its least important part.
#
# Bytes, not characters, for the reason the driver counts bytes: a byte count
# is never lower than a rune count, so a body inside this bound is inside
# GitHub's limit whichever unit that limit turns out to count.
MAX_BODY_BYTES: 65536
# Held back from the note's budget for the truncation notice, which is
# written after the cut point is chosen and so cannot be measured before it.
NOTICE_BYTES: 256
run: |
set -euo pipefail
body="$MARKER"$'\n'"$(cat "$VERDICT")"
# A reader who sees no inline comment for an observation would otherwise
# have to guess whether it was dropped.
#
# Bounded against what the marker and the verdict already spend, so the note
# can never be the thing that pushes the comment past GitHub's cap. Cut on
# whole lines: a byte cut can land inside a UTF-8 sequence or inside a
# finding's markdown, where a line cut can be counted and named.
if [ -n "$NOTE" ] && [ -s "$NOTE" ]; then
body="$body"$'\n\n'"$(cat "$NOTE")"
room=$(( MAX_BODY_BYTES - $(printf '%s\n\n' "$body" | wc -c) ))
if [ "$(wc -c < "$NOTE")" -le "$room" ]; then
body="$body"$'\n\n'"$(cat "$NOTE")"
else
# Every line the place step writes for a finding opens with "- `".
total="$(grep -c '^- `' "$NOTE" || true)"
budget=$(( room - NOTICE_BYTES ))
kept=""
shown=0
if [ "$budget" -gt 0 ]; then
# sed drops the last line, which is the one the byte cut may have left
# half-written.
kept="$(head -c "$budget" "$NOTE" | sed '$d')"
shown="$(printf '%s\n' "$kept" | grep -c '^- `' || true)"
fi
if [ "$shown" -gt 0 ]; then
body="$body"$'\n\n'"$kept"
fi
body="$body"$'\n\n'"_Cut to stay inside GitHub's comment limit ($MAX_BODY_BYTES bytes): $shown of $total observation(s) are shown here. All $total are in this workflow run's log._"
echo "::warning::the summary note was cut to $shown of $total unplaced observation(s) to fit GitHub's comment limit; the full list follows"
cat "$NOTE"
fi
fi
# One bot comment per PR: find by marker -> PATCH, else POST. repo/pr from env,
# not template-interpolated into the script.
id="$(gh api "repos/$REPO/issues/$PR/comments" --paginate \
--jq "map(select(.body | startswith(\"$MARKER\"))) | .[0].id // empty")"
if [ -n "$id" ]; then
gh api -X PATCH "repos/$REPO/issues/comments/$id" -f body="$body" >/dev/null
#
# Each call's failure is caught rather than left to `set -e`, so this step can
# say what was lost before it ends. A failed lookup does not fall through to
# POST: that leaves a second sticky comment behind, and one comment per pull
# request is this tool's whole contract with the reader.
posted=false
if id="$(gh api "repos/$REPO/issues/$PR/comments" --paginate \
--jq "map(select(.body | startswith(\"$MARKER\"))) | .[0].id // empty")"; then
if [ -n "$id" ]; then
if gh api -X PATCH "repos/$REPO/issues/comments/$id" -f body="$body" >/dev/null; then posted=true; fi
else
if gh api -X POST "repos/$REPO/issues/$PR/comments" -f body="$body" >/dev/null; then posted=true; fi
fi
fi
if [ "$posted" = true ]; then
echo "posted the verdict on $REPO#$PR"
exit 0
fi

# continue-on-error above is what stops a publishing failure from discarding a
# review that ran. It also removes the only signal there was, and a review that
# is silently absent is worse than a red job. So the failure is stated in two
# places, because neither alone is enough: an annotation on the run, which needs
# nothing but the runner; and the check run, which is the only one of the two
# that reaches the pull request, where the reader is waiting for a review that
# is not coming.
#
# The verdict goes to the log unposted, so the run still holds what the review
# cost model spend and a sandbox to produce.
bytes="$(printf '%s' "$body" | wc -c | tr -d '[:space:]')"
echo "::error::the review reached a verdict but it could not be posted on $REPO#$PR ($bytes bytes); it is in this step's log below"
echo "--- verdict, unposted ---"
printf '%s\n' "$body"

@github-actions github-actions Bot Sep 4, 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 dumps model-generated prose (which is derived from untrusted PR content) straight to the runner's stdout. Any line in the verdict that starts with :: is parsed by the runner as a workflow command — ::add-mask:: would replace arbitrary later log text with ***, ::stop-commands::<tok> would neutralise the workflow commands that follow (including the warning on line 1205), and ::notice::/::error:: would forge annotations on the run.

Cheap fix: bracket the dump with a random token, e.g.

tok="stop-$RANDOM$RANDOM"
echo "::stop-commands::$tok"
printf '%s\n' "$body"
echo "::$tok::"

The same applies to cat "$NOTE" on line 1153, which prints Finding.Detail verbatim.

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.
if [ -n "${REVIEWED_SHA:-}" ]; then
gh api -X POST "repos/$REPO/check-runs" \
-f name=review \
-f head_sha="$REVIEWED_SHA" \

@github-actions github-actions Bot Sep 4, 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] REVIEWED_SHA and $REPO can refer to different repositories here. This step's REPO (line 1098) falls back to github.repository when there is no App token, while steps.head always resolves the sha against needs.guard.outputs.review_repo || github.repository. So on a cross-repository review without App identity, this posts a check run to the asking repo with a head sha that only exists in the reviewed repo — GitHub answers 422 and the fallback warning on line 1205 fires every time. The check-run half of the signal is then structurally dead in exactly that path.

The sibling Publish the review check run step avoids this because its REPO uses the same expression as steps.head. Either gate this POST on the sha and repo agreeing (steps.identity.outputs.token != '' or a same-repo review), or record the asking PR's head sha as well and use that when REPO fell back.

-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 \
|| echo "::warning::the failure check run could not be posted either; the annotation on this run is the only record"
else
gh api -X POST "repos/$REPO/issues/$PR/comments" -f body="$body" >/dev/null
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
Loading