From ff34d3ff2d157acbb7fbee4b33452053c4099f08 Mon Sep 17 00:00:00 2001 From: bdchatham Date: Fri, 4 Sep 2026 13:44:33 -0700 Subject: [PATCH 1/2] fix(seidroid-review): fail soft when the step cannot post the verdict The sticky upsert was the only publish step that could fail the job. The five steps around it all carry continue-on-error: read-threads, record-commit, place-findings, publish-check-run and state-position. They carry it because a publishing failure must not bury a review that ran. This step carried none, and it is the last step in the job. A failure there threw the whole review away: the model spend, the sandbox run, and the verdict the reader waits for. continue-on-error alone makes that failure silent, which is worse than a red job. Two things now carry the signal, because neither one is enough alone: - An ::error:: annotation, written from inside the step. It survives continue-on-error, because that flag changes the step's conclusion and not a workflow command. It needs nothing but the runner. It also prints the unposted body to the log, so the run still holds the verdict. - A check run named `review`, with conclusion failure. Nobody opens a green run, so the annotation alone never reaches the reader. The name matches the check the step above publishes. That later check supersedes the green one on the same commit, and a run that does post clears it again. The same rewrite bounds $NOTE against what the marker and the verdict already spend. The driver clips the verdict it writes (review.MaxBodyBytes). Nothing clipped $NOTE, which carries raw model prose with no length limit on it. Up to 50 findings can reach it. The usual route into it is by design: a finding on a file the pull request does not touch has nowhere to go. Unbounded, that tail pushes the body past GitHub's comment limit. It then loses a whole review over its least important part. The note is now cut on whole lines, and the comment names how many observations it shows. MARKER is unchanged, so every open pull request keeps its sticky comment. This change leaves permissions alone, declares no new job output, and adds no step. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/seidroid-review.yml | 106 ++++++++++++++++++++++++-- 1 file changed, 100 insertions(+), 6 deletions(-) diff --git a/.github/workflows/seidroid-review.yml b/.github/workflows/seidroid-review.yml index 18c954b..d80063f 100644 --- a/.github/workflows/seidroid-review.yml +++ b/.github/workflows/seidroid-review.yml @@ -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 shell: bash env: GH_TOKEN: ${{ steps.identity.outputs.token || github.token }} @@ -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" + 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" \ + -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 From d551628fb713664b4867f4168a56885bc9e79eab Mon Sep 17 00:00:00 2001 From: bdchatham Date: Fri, 4 Sep 2026 14:26:53 -0700 Subject: [PATCH 2/2] feat(seidroid-review): state the finding counts on the verdict comment The verdict comment carried prose and nothing countable. A reader had to open the check run to learn how many findings the review made, and had to scroll the diff to learn how many reached a line. ai-review prints one line that answers both. Both reviewers run during the transition, so a reader moves between them and should read one format. This adds that line under the review's prose: **Findings:** 2 blocking | 3 non-blocking | 5 posted inline | 1 pre-existing The counts are corrected, not copied. ai-review has two defects here and this does not reproduce them. First, its blocking total adds pre-existing blockers. seidroid's CheckConclusion excludes pre-existing from the gate on purpose: the code is already on the base branch, so a gate on it fails every pull request that touches the file, and the author who must clear the check is the one person who did not cause it. A total that folds them in prints a number the check run contradicts. So pre-existing is counted apart and named. Folded into "non-blocking" instead, it would tell an author their change has problems it does not have. Second, its non-blocking total counts only anchored comments, so an orphaned suggestion is dropped from the total but still shown. Ours reads the driver's own count over every reported finding, anchored or not. The counts come from the driver rather than from a second pass over the findings. The driver derived the check run's conclusion from those same findings, and a second derivation is a second thing that can disagree. Where the two do disagree the step says so in the log and still prints what it was given: correcting a number here would publish a third answer and hide the defect that produced the first two. Two supporting changes carry it: The place step now writes on_line, on_file and unplaced to $GITHUB_OUTPUT. It writes them at exactly two points -- the early exit for "nothing to place", as literal zeros, and the end of the run block. A run that dies between them leaves them unwritten, and unwritten is correct there: placement neither finished nor was skipped, so no number it could publish would be true. All three go in one append, so a reader never sees a half-written set. The line is appended before the unplaced-observations note, and that ordering is load-bearing twice. It puts the line under the prose, where the format wants it. And the note's byte budget measures $body to decide what room is left, so a line added after that measurement is a line nothing accounted for -- and what it pushes past GitHub's 65,536-byte cap is the whole review. Nothing here can fail the publish. Every count is validated before use and every term is dropped rather than guessed. A driver that predates the check.json count fields makes jq answer null, the sentinel fails validation, and the comment ships with a shorter line that is still true. A skipped placement says "inline placement did not run" rather than claiming zero. When neither is readable the line is not written at all. MARKER is unchanged, so the sticky upsert still finds its prior comment. The line goes after it. Permissions are unchanged. Verified: the workflow parses; bash -n passes on all 11 run blocks; shellcheck is clean on both changed blocks and actionlint reports the same five pre-existing findings as the base. The place step was executed on all five of its paths, and the verdict step on the seven-case skew matrix, against a stubbed gh. All exit 0. The byte budget was exercised at caps of 900, 1200, 2000 and 65,536: the line survives, the note absorbs the cut, and the body stays under the cap. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/seidroid-review.yml | 106 ++++++++++++++++++++++++++ 1 file changed, 106 insertions(+) diff --git a/.github/workflows/seidroid-review.yml b/.github/workflows/seidroid-review.yml index d80063f..7635b71 100644 --- a/.github/workflows/seidroid-review.yml +++ b/.github/workflows/seidroid-review.yml @@ -882,6 +882,10 @@ jobs: : > "$NOTE" if [ ! -s "$FINDINGS" ]; then echo "no findings to place; the summary carries the review" + # Zero, and not silence. Nothing was placed because there was nothing to + # place, which is a number the summary can state. A step that was skipped + # writes nothing at all, and the summary tells the two apart by that. + { echo "on_line=0"; echo "on_file=0"; echo "unplaced=0"; } >> "$GITHUB_OUTPUT" exit 0 fi # The commit the review actually read, recorded before it started. Absent @@ -933,6 +937,14 @@ jobs: } > "$NOTE.tmp" mv "$NOTE.tmp" "$NOTE" fi + # Here and at the early exit above, and nowhere between: the two points where + # these are final. A run that dies in between leaves them unwritten, which is + # the right answer there -- placement neither finished nor was skipped, so no + # number it could publish would be true. + # + # One append for all three, so the summary never reads a half-written set. It + # requires all three for that reason, including the one no term renders. + { echo "on_line=$on_line"; echo "on_file=$on_file"; echo "unplaced=$unplaced"; } >> "$GITHUB_OUTPUT" echo "findings: $on_line on a line, $on_file on a file, $unplaced in the summary" - name: Publish the review check run @@ -1101,6 +1113,16 @@ 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 }} + # What the driver counted, for the findings line below. Read rather than + # recomputed here: the driver derived the check run's conclusion from these + # same findings, and a second derivation is a second thing that can disagree. + CHECK: ${{ steps.drive.outputs.check_path }} + # What the step above placed. Empty, not zero, when that step was skipped for a + # cross-repository review or died partway -- and the difference is the point. + # "0 posted inline" over a placement that never ran is a lie about the review. + ON_LINE: ${{ steps.place.outputs.on_line }} + ON_FILE: ${{ steps.place.outputs.on_file }} + UNPLACED: ${{ steps.place.outputs.unplaced }} # 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. @@ -1122,6 +1144,90 @@ jobs: run: | set -euo pipefail body="$MARKER"$'\n'"$(cat "$VERDICT")" + + # The findings line, in the shape ai-review posts, so a reader moving between + # the two reviewers during the transition reads one format. Assembled here + # rather than in the driver because half of it is the placement above, which + # the driver never sees. + # + # It is appended before the note block below, and that ordering is load-bearing + # twice. It puts the line under the review's prose, where the format wants it. + # And the note's byte budget measures $body to decide what room is left, so a + # line added after that measurement is a line nothing accounted for -- and what + # it pushes past GitHub's cap is the whole review. + # + # Every term is dropped rather than guessed, and nothing here can fail the + # publish. A count this run cannot read costs the term; a line with no count + # left in it is not written at all. + is_count() { case "${1:-}" in (''|*[!0-9]*) return 1 ;; esac; } + line="" counted=false + add_term() { if [ -n "$line" ]; then line="$line | "; fi; line="$line$1"; } + + # The driver's own totals, over every finding the review reported rather than + # over the ones that could be placed: a blocker naming no line is still a + # blocker, and a count that omitted it would read as a cleaner review than the + # one that ran. + # + # A driver older than these fields writes none of them and jq answers null. The + # sentinel makes that a value is_count rejects, so an old driver publishes the + # same comment with a shorter line that is still true. + blocking="" non_blocking="" pre_existing="" conclusion="" + if [ -s "${CHECK:-}" ]; then + read -r blocking non_blocking pre_existing conclusion < <(jq -r '[(.blocking // "?"), + (.non_blocking // "?"), (.pre_existing // "?"), (.conclusion // "?")] | @tsv' \ + "$CHECK" 2>/dev/null) || true + fi + if is_count "$blocking" && is_count "$non_blocking"; then + add_term "$blocking blocking" + add_term "$non_blocking non-blocking" + counted=true + else + echo "::notice::this driver reports no finding counts, so the findings line \ + omits them; it predates check.json's blocking and non_blocking fields" + fi + + # Both placements the step above makes: on a line where the diff carries it, on + # the file where it does not. Both land in the pull request's file view, which + # is what "inline" means to the person reading it. What reached neither is not + # counted here -- the note below names those one by one under a heading of their + # own, which is more than a number would say. + if is_count "${ON_LINE:-}" && is_count "${ON_FILE:-}" && is_count "${UNPLACED:-}"; then + add_term "$(( ON_LINE + ON_FILE )) posted inline" + counted=true + else + add_term "inline placement did not run" + echo "::notice::no placement counts, so the findings line says so; that step \ + is skipped for a cross-repository review and writes nothing if it dies" + fi + + # Counted apart from both, and named. CheckConclusion excludes a pre-existing + # blocker from the gate on purpose -- it is already on the base branch, so + # failing on it would fail every pull request that touches the file, and the + # author who has to clear the check is the one person who did not cause it. + # Folded into "blocking" it would print a number the check run contradicts; + # folded into "non-blocking" it would tell an author their change has problems + # it does not have. Omitted at zero, which is most reviews. + if is_count "$pre_existing" && [ "$pre_existing" -gt 0 ]; then + add_term "$pre_existing pre-existing" + fi + + # Two numbers about one review must not disagree. Both are the driver's, from + # the same findings, so a disagreement is a defect there rather than a case to + # render around. It is stated in the log and the line still prints what it was + # given: correcting either number here would publish a third answer and hide + # the bug that produced the first two. + if is_count "$blocking" && [ "$conclusion" = "failure" ] && [ "$blocking" -eq 0 ]; then + echo "::warning::the driver reported 0 blocking findings beside a failing \ + check; this comment and the check run disagree" + elif is_count "$blocking" && [ "$conclusion" != "failure" ] && [ "$blocking" -gt 0 ]; then + echo "::warning::the driver reported $blocking blocking finding(s) beside a \ + $conclusion check; this comment and the check run disagree" + fi + + if [ "$counted" = true ]; then + body="$body"$'\n\n'"**Findings:** $line" + fi + # A reader who sees no inline comment for an observation would otherwise # have to guess whether it was dropped. #