feat(seidroid-review): fail soft when the verdict cannot post, and state the finding counts - #71
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>
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) <noreply@anthropic.com>
PR SummaryLow Risk Overview Place findings writes The line is inserted before the unplaced-note byte budget: blocking/non-blocking from Reviewed by Cursor Bugbot for commit d551628. 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.
Adds a **Findings:** … line to the sticky verdict comment, sourced from the driver's check.json counts plus the place step's new on_line/on_file/unplaced outputs. The logic is defensive and correct — every value is validated before use, missing values drop their term rather than being guessed, and the line is appended before the note's byte-budget calculation so the budget accounts for it; only minor wording and cross-tool consistency notes.
Findings: 0 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
N non-blockingcomes from the driver's own total (which the PR description says is taken after a nit filter) whileP posted inlinecounts every comment the place step posted, including nits. If findings.json still carries nits,posted inlinecan exceedblocking + non-blockingand read to a reader as an arithmetic error. Worth confirming the driver applies the same filter to both, or the line could sayN blocking | M non-blocking (+K nits).- Merge-order hazard the author already flagged: the findings line must stay above the
room=$(( MAX_BODY_BYTES - ... ))computation from #70. If #70 is squashed or rebased and this is replayed onto the result, verify that ordering survived — a findings line appended after the measurement is unaccounted bytes that can push the whole comment past GitHub's 65,536-byte cap. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| add_term "$(( ON_LINE + ON_FILE )) posted inline" | ||
| counted=true | ||
| else | ||
| add_term "inline placement did not run" |
There was a problem hiding this comment.
[suggestion] "did not run" overstates what the empty outputs prove. The place step is continue-on-error: true and writes all three outputs only at its two terminal points, so a run that dies partway — say the gh api .../pulls/$PR --jq .head.sha read fails, or a base64 --decode trips set -e mid-loop — can have posted several inline comments and still leave ON_LINE/ON_FILE/UNPLACED empty. The reader then sees seidroid comments in the file view beside a summary saying placement never happened, which is the same class of false statement the surrounding comments are careful to avoid elsewhere. "inline placement did not complete" (or "…count unavailable") covers both the skip and the partial-death case without asserting the stronger claim.
bdchatham
changed the title
|
Retargeted to Both are the same concern — how a review reaches the pull request — so they review better as one unit than as a stack against an unmerged branch. The prompt schema change is deliberately NOT here. An adversarial pass returned UNSAFE on backward compatibility, so the |
1 participant
What this does
The verdict comment carries prose and nothing countable. A reader has to open
the check run to learn how many findings the review made, and scroll the diff
to learn how many reached a line.
ai-reviewprints one line that answersboth. 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:
MARKERis unchanged, so the sticky upsert still finds its prior comment.Permissions are unchanged. The diff is 106 added lines and no deleted ones.
The counts are corrected, not copied
ai-review's counts have two defects. This does not reproduce either.Its blocking total adds pre-existing blockers. seidroid's
CheckConclusionexcludes 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 — "3 blocking"
beside a neutral check. So pre-existing is counted apart and named. Folded into
non-blockinginstead, it would tell an author their change has problems itdoes not have.
Its non-blocking total counts only anchored comments. It computes
inlineOther = reviewComments.length - inlineBlockers, so an orphanedsuggestion is dropped from the total while still being shown. Ours reads the
driver's own count over every reported finding, anchored or not.
N blockingcheck.json .blockingblockersbucket. Excludes pre-existing, which is what makes it agree withCheckConclusion.M non-blockingcheck.json .non_blockingnon_blockersplus every reported non-blocking finding, anchored or not, after the nit filter.P posted inlineon_line + on_filefrom the place stepK pre-existingcheck.json .pre_existingunplacedis exported but not rendered: the note section below already nameseach one under its own heading, which says more than a number. It is required
to be present as proof the output set is whole.
N and M count findings; P counts comments. They measure different things and
may legitimately differ — dedupe, the 50-comment placement cap, the nit filter.
The labels say which is which.
Where the numbers come from, and why not from here
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 emits a
::warning::and still prints whatit was given. Correcting a number here would publish a third answer and hide
the defect that produced the first two.
Two supporting changes
The place step exports its counters.
on_line,on_fileandunplacedgoto
$GITHUB_OUTPUTat exactly two points: the early exit for "nothing toplace", 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 brace-group append, so a consumer never reads a half-written
set. I rejected a
trap ... EXITthat emits partial counts: it would report"3 posted inline" for a step that crashed after 3 of 10, fires on the early-exit
path too, and clobbers
$?.The line is appended before the note block, and that ordering is
load-bearing twice. It puts the line under the prose, where the format wants
it. And #70's note budget computes
roomfromwc -cof$body, so a lineadded after that measurement is a line nothing accounted for — and what it
pushes past GitHub's 65,536-byte cap is the whole review. Inserting before the
block means #70's arithmetic absorbs the change with no edit to it.
Nothing here can fail the publish
Every value is validated by
is_count()before use, and every term is droppedrather than guessed. The measured matrix:
2 blocking | 3 non-blocking | 5 posted inline | 1 pre-existing2 blocking | 3 non-blocking | inline placement did not run | 1 pre-existing+::notice::5 posted inline+::notice::— shorter, still true, still published::notice::only0 blocking | 0 non-blocking | 0 posted inline— every zero earnedcheck.jsonmissing, empty or malformedon_fileempty)The jq sentinel is
// "?"rather than// 0, because a numeric sentinel isindistinguishable from a real count.
"?"failsis_countby construction.jq's
//treats onlynullandfalseas falsy, so a legitimate0passesthrough untouched.
The driver contract this waits on
check.jsontoday carries{conclusion, title, summary}only — verifiedagainst
internal/review/check.go. The three count fields do not exist yet. Soon merge this ships in the old-driver row of the table above: the comment gains
**Findings:** N posted inlineand a::notice::. It is correct and it ispublished; it simply says less until the driver catches up.
The driver needs to add three JSON integers named
blocking,non_blockingand
pre_existing, in the existing bare-noun style.One thing for the driver author to resolve.
CheckConclusionreturnsfailurewhenDecision() == "request_changes", independently of any count. Areply that requests changes with an empty
blockersarray and noblocker-severity finding yields
conclusion: failurewith a natural count of0. That posts
REQUEST_CHANGESbeside**Findings:** 0 blocking, and tripsthe disagreement warning on a legitimate review. The workflow deliberately does
not paper over it — see above — so the driver has to decide whether the
escalation contributes to the count or the count carries a separate term.
Known limitations, stated rather than fixed
RenderCommentreturns
prose + footer, and the workflow appends after that. The paritytarget puts the line directly under the prose. Moving it requires
RenderCommentto own the line, which needs the placement counts the drivernever sees.
### Blockers/### Non-blocking/### Pre-existing issuessectionsare not in scope here. They exist today only in
checkSummary, on the checkrun. The workflow has integers, not buckets, and cannot render them. Adding
them to the comment also imports
checkSummary's threat model intopublish.go— model text under this tool's headings needsdefuseMarkup,and
check_test.go:286already fixtures a non-blocker that forges a### Blockingsection. That belongs in a driver change with that guard.RenderCommentpostsv.Textverbatim and does not defuse it. A verdict ending inside an unterminated code
fence renders what follows as code; a trailing
<!--hides it. The residualis bounded: the counts are driver integers and shell counters, so the worst
case is a suppressed line, never a forged one. A model cannot write itself
a clean Findings line.
maxPlaceableFindings = 50is invisible here. Findings past the cap neverreach
findings.json, so they reach neither the placement nor the note. N+Mcan exceed what is shown. The workflow cannot detect this: N+M legitimately
exceeds the placed count anyway, because the
blockersandnon_blockersbuckets are tied to no line. Surfacing it needs the driver.
Verification
python3 -c "import yaml; yaml.safe_load(...)"parses.bash -npasses on all 11 extractedrun:blocks.shellcheckis clean on both changed blocks.actionlintreports the samefive pre-existing findings as the base — this change adds none.
gh:empty findings, line placement, file fallback, full fallthrough to the note,
and a mid-run death. The mid-run death writes no keys, as designed.
warnings and the
null/ negative / non-numeric skew inputs. All exit 0. Noinput renders
null blocking.40-entry note: the marker stays first, the Findings line survives, the note
absorbs the cut, and the body stays under the cap.
None of this exercises a live GitHub API. The
ghcalls are stubbed.