fix(seidroid-review): fail soft when the step cannot post the verdict - #70
fix(seidroid-review): fail soft when the step cannot post the verdict#70bdchatham wants to merge 1 commit into
Conversation
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) <noreply@anthropic.com>
PR SummaryMedium Risk Overview Unplaced findings ( Posting is defensive: comment lookup and PATCH/POST errors are handled explicitly (no POST after a failed lookup, avoiding duplicate stickies). On publish failure the step emits an Reviewed by Cursor Bugbot for commit ff34d3f. Bugbot is set up for automated code reviews on this repo. Configure here. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sensible fail-soft change: the sticky-verdict step no longer discards a completed review when publishing fails, and it bounds the unplaced-findings note so the note can't push the comment past GitHub's 65,536-byte cap. The truncation arithmetic checks out; three non-blocking gaps remain around the replacement failure signal (cross-repo check-run target, uncovered set -e aborts, and raw untrusted output in the runner log).
Findings: 0 blocking | 5 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- A
PATCHthat returns 404 (the sticky comment was deleted between the list and the patch) is treated as a publishing failure rather than falling back toPOST. The comment at lines 1159-1162 reasons about not falling through after a failed lookup, which is correct, but a 404 on PATCH is the one case where a POST is provably safe — the marker comment no longer exists, so no duplicate can result. - Agreeing with the description's own caveat on the
reviewcheck-run name: besides the merge-gate change, the PR's checks list will show two entries namedreview(the earlier green from the publish step and the new red) on the same commit; only the latest counts for branch protection, but a reader sees both. Worth confirming supersession on a throwaway PR before merge, as the description suggests. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| if [ -n "${REVIEWED_SHA:-}" ]; then | ||
| gh api -X POST "repos/$REPO/check-runs" \ | ||
| -f name=review \ | ||
| -f head_sha="$REVIEWED_SHA" \ |
There was a problem hiding this comment.
[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.
| # 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 |
There was a problem hiding this comment.
[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).
| 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" |
There was a problem hiding this comment.
[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.
|
Closing in favour of a single PR carrying the whole change. The fail-soft publish fix here is correct and stands — it is not being abandoned. It is being folded into one branch alongside the Findings-line work (correct-count The branch |
1 participant
Why
The
Post verdict (sticky upsert)step was the only publish step in thereviewjob that could fail the job. Its five neighbours all carrycontinue-on-error, because a publishing failure must not bury a review thatran:
continue-on-errorIt is also the last step in the job, and it runs
set -euo pipefail. A failedgh apicall there threw the whole review away: the model spend, the sandboxrun, and the verdict itself. On the PATCH branch the outcome is worse than a
lost review, because the previous run's comment stays on the pull request. A
reader then sees a stale verdict presented as the current one.
What now carries the signal
continue-on-erroralone would make the failure silent, and a review that isquietly absent is worse than a red job. Two things replace the signal, because
neither one is enough alone:
::error::annotation, written from inside the step. It survivescontinue-on-error, because that flag changes the step's conclusion and nota workflow command. It needs nothing but the runner. It also prints the
unposted body to the log between
--- verdict, unposted ---markers, so therun still holds what the review cost to produce.
review, conclusionfailure. Nobody opens a greenrun, so the annotation alone never reaches the reader. This check uses the
same name as the one the step at line 938 publishes. A later check run of
that name supersedes the earlier green on the same commit. The pull request
then goes red for a review that is not there, and a re-run that does post
clears it again. The POST is best effort: whatever stopped the comment can
stop it too, and then the annotation stands alone.
The
$NOTEboundThe driver clips the verdict it writes (
review.MaxBodyBytes, 60,000). Nothingclipped
$NOTE, which carriesFinding.Detail— raw model prose with nolength limit on it. The step now bounds
$NOTEagainst what the marker and theverdict have already spent, and cuts it on whole lines rather than bytes.
A byte cut can land inside a UTF-8 sequence, or inside a finding's markdown. A
reader can count and name a line cut instead. The comment then states how many
observations it shows, a
::warning::repeats the counts, and the full listgoes to the log.
A note that fits produces a byte-identical body to today. That is the
normal case, and the table below asserts it.
What the 422 claim got right, and what it did not
The claim that motivated this bound is partly correct. I am implementing
what survived and stating what did not.
Holds. The arithmetic is right: a verdict at its 60,000-byte ceiling plus
the 25-byte prefix leaves 5,511 bytes for
$NOTE. The failure path is real,and it is more reachable than the claim described. The place step wraps both
gh apicalls asif gh api ... >/dev/null 2>&1. Any non-zero exittherefore sends a finding to
$NOTE, not only a size 422. A secondary rate limitacross a 50-comment loop is exactly the right shape to trigger it.
Does not hold. The claim treated 5,511 bytes as the threshold at which a
single oversized
Finding.Detailstarts the cascade. It is not.A pull request review comment carries the same ~65,536-character body limit as
an issue comment. A detail must therefore exceed roughly 65,536 characters to
422 the line post on size. A detail between 5,511 and 65,536 bytes posts
cleanly on the line, and never reaches
$NOTE. 5,511 is only the room thatremains once a note already exists.
Also corrected. 5,511 bytes is the worst case, not the standing margin. The
margin narrows that far only when the verdict sits at its truncation ceiling. On a typical
few-kilobyte verdict the margin is tens of kilobytes. And
MARKERis 24 bytes,not 25; the 25th byte is the newline the shell inserts after it.
The dominant route into
$NOTEis neither of those. It is the designed one.The workflow says so in its own comment at line 856.
The bound earns its place because
findings accumulate there, not because one field runs away.
Validation
Everything below ran against this commit.
yaml.safe_loadsucceeds. Jobs are stillguardandreview. Thereviewjob's permissions still read exactlypull-requests: write,contents: read,checks: write.run:blocks in the file passbash -n.SC2102:info, and nothingelse. They are the
-f output[title]=/-f output[summary]=construct thatthe existing check-run step already emits at line 956. No new class, nothing
above
info.ghthat records everyargument and fails on a configurable pattern. Fixtures: a 60,000-byte verdict
and a 122,679-byte note of 400 findings in the place step's exact format.
::error::, body dumped, check runname=review conclusion=failure, exit 0::error::+ check run$NOTEunset::error::+::warning::, zero check-run calls::error::+::warning::, exit 0LC_ALL=C/C.UTF-8/en_US.UTF-8Not run, and not claimed. No real GitHub API call. The 65,536 rejection
threshold comes from GitHub's documented limit, not from measurement.
The check-run supersession also comes from GitHub's documented semantics. A
second check run of the same name on the same commit wins in the merge box. No
one has watched that happen on a live pull request. That is the load-bearing
half of the signal. Confirm it on a throwaway PR before this merges.
What I deliberately did not change
MARKER. A one-way door. The upsert finds its prior comment bystartswith("$MARKER"), so changing it strands the sticky comment on everyopen pull request.
checks: write. Thereviewjob already declares it at line 579, and the step already carriesthe same token the existing check step uses.
sei-agent-driver.grep -c '^-'matches the place step's own format, but aFinding.Detailcontaining such a line inflates both counts. The effect is cosmetic — the notice may say "14 of 31" when the truth is "14 of 29". An exact count needs the place step to writeunplacedto$GITHUB_OUTPUT`, which it does not do today. Clean follow-up, not thisPR.
The one behaviour change worth a reviewer's attention
The
reviewcheck run can now go red for a publishing failure, not onlyfor a code finding. If branch protection ever requires the
reviewcontext, afailed comment post now blocks merge. Before, it only reddened the job. This is
deliberate: the check is the durable signal that replaces the red job. It
supersedes the earlier green on the same commit, and a successful re-run clears
it.
If you would rather leave the merge gate untouched, change
-f name=reviewtoa distinct name such as
review · publish. The cost is a check that neverclears itself, because nothing re-posts it green.
🤖 Generated with Claude Code