feat(seidroid-review): post the findings as one review - #97
Conversation
PR SummaryMedium Risk Overview Batch failure handling is explicit: 4xx (e.g. 422/413) retries each finding down the per-line → file → summary ladder; other errors (5xx, no status line) send batch findings to the summary to avoid duplicate reviews. Fixes include empty-field @TSV shift and coercing non-numeric Adds Reviewed by Cursor Bugbot for commit b7e9f8d. 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.
Solid, well-reasoned consolidation of per-finding comments into one createReview call, with a sensible three-rung degradation ladder and two real base-branch bugs (the @tsv field shift and the unvalidated -F line=) fixed along the way. Three non-blocking issues: a summary header that misdescribes findings routed there by a batch failure, 422 detection resting on an undocumented status field, and findings in files whose patch GitHub omits getting silently demoted off their line.
Findings: 0 blocking | 3 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
| place_each place_one < <(rows '.anchored[]' "$placement") | ||
| else | ||
| echo "::warning::the review carrying $anchored comment(s) could not be posted on $REPO#$PR; those findings are in the summary instead of on the diff" | ||
| place_each to_summary < <(rows '.anchored[]' "$placement") |
There was a problem hiding this comment.
[suggestion] Findings routed here are on lines the diff does cover, but they land under the wrong header. Reaching this branch requires head_sha to be non-empty (it gates batched), so the block at 1764-1769 picks **Observations off the changed lines.** These are about code this pull request does not touch, so there is nowhere in the diff to attach them:. For a 500 or a dropped connection on the batch, every anchored finding is described to the author as being about untouched code — exactly the mismatch the two-header split at 1759-1763 was added to avoid.
Concrete case: 4 findings all on added lines, createReview returns 503. All four go to the note, and the author reads that all four are outside their diff.
A flag set in this branch selecting a third header (e.g. "the review carrying these could not be posted on the diff") would keep the note saying something true about the reader's own change.
There was a problem hiding this comment.
Fixed in cabbc50. Those findings now go to a group of their own, headed **On the changed lines, and not posted.** GitHub would not take the review carrying these, so they are here rather than on the lines they name:. Both groups still count as unplaced, so the resolve gate is unchanged.
Harness cases 15, 15b and 15c assert the two headings, their order, the count under each, and that no on-diff group means no on-diff heading.
| on_line=$((on_line + anchored)) | ||
| echo "posted one review carrying $anchored comment(s) on $REPO#$PR" | ||
| else | ||
| status="$(jq -r '.status // empty' "$response" 2>/dev/null || true)" |
There was a problem hiding this comment.
[suggestion] status is not part of GitHub's documented validation-error (422) schema — the comment at 2008-2012 says as much for the position step — and the PR body lists the 422 shape as unverified. The two steps make opposite bets on an absent code, though: there, absent means "no downgrade, keep the vote", which is safe. Here, absent means every anchorable finding drops two rungs straight to the summary, which is strictly worse than base (base would have placed them one by one on their lines), and unplaced > 0 then holds every superseded thread open at 2644, so the author sees each restated finding twice.
Concrete case: createReview 422s on one bad line with body {"message":"Validation Failed","errors":[...]} and no status. Ten findings that base would have placed on nine lines and one file all land in the summary, and no superseded thread closes.
Reading the code off the response rather than the body would be robust without depending on gh's human-facing stderr line: gh api -i -X POST ... > "$response" and take the code from the status line, or fall back to treating a body carrying an errors array as a refusal.
There was a problem hiding this comment.
Fixed in 37b2edf, and you are right that the failure direction was the wrong one. The code now comes from the response's own status line via gh api -i, not from a body field. Measured on this endpoint, a 422 does carry status today — and the schema does not promise it, which is your point.
An absent code still takes the summary, because that is the case where the write may have landed. Case 16 sends a 422 whose body has no status and asserts the ladder runs: 4 line retries, on_line 4, unplaced 1.
| # rather than met there. | ||
| # shellcheck disable=SC2016 # $findings and friends are jq's, and single quotes are what keep them jq's | ||
| partition='def hunks: | ||
| reduce ((.patch // "") | split("\n"))[] as $l |
There was a problem hiding this comment.
[suggestion] (.patch // "") silently turns a file with no patch into a file with no commentable lines. pulls.listFiles omits patch when a file's diff is too large (and for binary files), and caps the response at 3000 files — so for those files every finding becomes loose regardless of whether its line is genuinely in the diff.
That costs twice: the finding drops from the line rung to the file rung, and the file-level body at 1600 then tells the author _Cited at `path:120`, outside this diff's changed lines._, which is false — the line is in the diff, the index just could not see it. Base would have posted it on the line, because it let the API decide.
Since the batch only needs to avoid lines the API will refuse, a file whose patch is unknown is better treated as unknown than as empty: keep those findings out of the batch but send them through place_one rather than on_file_or_summary, so the API still gets the chance to accept the line.
There was a problem hiding this comment.
Fixed in 37b2edf. A file the API sends without a patch is now a third group rather than a file with no lines: those findings stay out of the batch and go through place_one, so the API still gets to accept the line.
Case 17 puts a finding at pkg/huge.go:120 behind an absent patch and asserts it lands on its line — one line call, no file-level comment, and no outside this diff's changed lines note that would have been false.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Placement posted one `POST /pulls/{pr}/comments` per finding, so an author
with ten findings read ten ungrouped threads and got ten notifications. The
findings the diff can hold now ride in a single `pulls/{pr}/reviews` call
carrying the review body and every inline comment together, which is how
`ai-review.yml` posts.
The three rungs hold. A line the diff covers goes on the line, in the batch.
A line the diff does not cover goes on its file, which the reviews API has no
field for and which therefore keeps a call of its own. A finding that reaches
neither travels in the summary. `on_line`, `on_file` and `unplaced` count the
same events and reach the verdict comment and the resolve gate unchanged.
The batch is all-or-nothing, so the set of commentable lines is read from the
pull request's own diff before the call rather than discovered by refusal. A
422 retries each finding on its own; any other failure sends those findings to
the summary, because a write that may have landed must not be repeated.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… the summary groups
Two claims the placement step made that were not true.
The index and the comments named different commits. The index came from
`GET /pulls/{n}/files`, which answers for the pull request's current head and
takes no commit — it accepts a `sha` parameter and ignores it, measured. Every
comment names the recorded `REVIEWED_SHA`. A push during the review therefore
marked lines from one diff and commented on another: a line the older commit
cannot carry costs the whole batch a 422, and a line it can carry is hidden and
falls to the file rung. The index now comes from
`GET /compare/{base.sha}...{REVIEWED_SHA}`, which does take a commit and whose
three-dot form is the diff the pull request shows. The check run is published
against the commit the review read for the same reason.
A failure that was not a 422 filed on-diff findings under a heading that denied
them. They went to the summary under "Observations off the changed lines. These
are about code this pull request does not touch", and they were on changed
lines. They now carry their own heading, which says GitHub would not take the
review that would have posted them. Both groups still count as unplaced, so the
resolve gate is unchanged.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f6eb66f to
cabbc50
Compare
…t an absent patch as unknown Two more claims the placement step made that were not true. It read the refusal code from a `status` field in the error body. GitHub's validation-error schema does not declare that field, so a 422 that carries none read as "not 422" and sent every anchorable finding to the summary — worse than the per-finding ladder this rung exists to reach, and `unplaced > 0` then holds every superseded thread open. The code now comes from the response's own status line, which `-i` puts first. A call that reached no response leaves no status line, and that empty code still takes the summary, because that is the case where the write may have landed. It counted a file the API sends without a patch as a file with no commentable lines. A binary file and a file whose diff was too large to send both arrive that way, so every finding on one dropped to the file rung under a body reading `Cited at path:120, outside this diff's changed lines` — which was false. Those findings now form a third group and go to the API one at a time, which is the only thing that can still say whether the line is in the diff. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 37b2edf. Configure here.
There was a problem hiding this comment.
Solid, carefully reasoned change that collapses N per-finding comment calls into one createReview batch, with a correct hunk walk, correct fallback ladder, and counters that stay in the parent shell. Two non-blocking gaps: files missing from a truncated compare response are misclassified as off-diff, and only 422 takes the safe per-finding retry.
Findings: 0 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The new partition/batch logic is the most intricate shell+jq in this workflow, and the verification harness described in the PR body (extracted step script +
ghstub) is not committed, so none of it is exercised in CI. Landing that harness — or even ajq-only unit check ofpartitionagainst fixture patches — would make the hunk walk and the three-way bucketing regression-proof. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion] A finding with an empty
fileis dropped without being counted as unplaced (base:[ -z "$path" ] && continuein the placement loop; the PR preserves the behaviour viaselect(.file != "")). BecauseResolve the threads this review closedgates superseded threads onunplaced == 0, such a finding can silently take a live thread off the pull request with nothing posted in its place.
| | ($findings[0] | ||
| | map(. as $g | ($index[$g.file]) as $h | ||
| | . + { ok: (($h[$g.side][$g.line | tostring]) // false), | ||
| unknown: (($index | has($g.file)) and ($h == null)) })) |
There was a problem hiding this comment.
[suggestion] The unknown bucket is keyed on $index | has($g.file), so a file that is in the pull request but missing from the compare response falls into loose instead. The compare endpoint truncates its files array on very large diffs — it does not page files the way GET /pulls/{n}/files does — so on a PR past that cap every finding in a truncated-away file skips the line rung entirely (loose goes straight to on_file_or_summary at line 1891) and gets a file comment whose body asserts _Cited at path:line, outside this diff's changed lines._. That is exactly the false statement the unknown bucket was added to avoid, and it is a regression from base, which always tried the line first and let the API decide.
A local guard would cover it: if $files | length is at the cap (or disagrees with the pull request's changed_files), treat the diff as unreadable and fall through to the per-finding path the elif at line 1836 already provides.
There was a problem hiding this comment.
Fixed in 3215d68. Your premise held on measurement, and the cap is 300 exactly.
I measured it rather than taking the value, because the guard rests on it. On kubernetes/kubernetes#137092, GET /pulls/{n} reports changed_files: 398 and GET /compare/{base}...{head} answers with exactly 300 — on a single page, so pagination cannot be the escape. GET /pulls/{n}/files --paginate returns all 398 on the same pull request, which is the asymmetry you named. Two more three-dot ranges of 309 and 321 files also answer with 300, and one of 251 answers with 251, so the list is whole below the cap.
--paginate does not help and is gone. Its pages are pages of commits: on a 2091-commit range the Link header offers 21 pages, page 1 carries 300 files, and pages 2, 3 and 21 carry no files key at all — has("files") == false. So the step now makes one call where it made up to one per hundred commits. Nothing signals the truncation either: no total, no Link for the files, no flag. per_page does not move the file list, only the commits array.
On the test: changed_files is the exact comparison and the cap test only carries the case where that count cannot be read.
if [ -n "$changed_files" ] && [ "$n" -ne "$changed_files" ]; then # short
if [ -z "$changed_files" ] && [ "$n" -ge 300 ]; then # at the cap, no count
I did not write it as "bail at 300 or on disagreement", because that costs a genuine 300-file pull request the batch for nothing. changed_files and the compare length agree on every whole list I measured, including 295 and 261 — just under the cap — so the equality is safe to rest on. Both fields come from the one GET /pulls/{n} call the step already made, so the guard costs no request.
Case 18 is the case that fails without the fix: pkg/b.go is in the pull request and missing from the compare response. Read as the diff it puts the finding on pkg/b.go under a body saying line 2 is outside a diff that adds line 2. Deleting the shortfall test breaks 6 assertions. Case 19 is the one that must not bail — 300 files with a count that confirms them — and making the cap test unconditional breaks 6 of its own.
Not verified: nothing ran on a GitHub runner. The read side above is measured against the live API; the write side is not.
| echo "posted one review carrying $anchored comment(s) on $REPO#$PR" | ||
| else | ||
| status="$(sed -n '1s|^HTTP/[0-9.]* \([0-9][0-9][0-9]\).*|\1|p' "$response" || true)" | ||
| if [ "$status" = "422" ]; then |
There was a problem hiding this comment.
[suggestion] Only 422 takes the per-finding retry. The most likely refusal for this particular call is an oversized request — it carries every finding's full detail, which is unbounded model prose — and GitHub can answer that with 413 rather than 422. On 413 (and on 400/401/403/404, none of which can be a partial write) every anchorable finding goes straight to the summary under a heading saying GitHub would not take the review, when posting them one at a time would likely have worked.
Matching any 4?? instead keeps only the genuinely ambiguous outcomes — 5xx, and the empty status from a call that reached no response — on the summary path, which is where the double-post risk the comment above describes actually lives.
There was a problem hiding this comment.
Fixed in 3215d68. Any 4xx now takes the per-finding retry.
case "$status" in
4??)
echo "::warning::$REPO#$PR refused the review carrying $anchored comment(s) with $status; ..."
place_each place_one to_summary_ondiff < <(rows '.anchored[]' "$placement") ;;
*)
... summary ...
esac
The status still comes from the response's own status line via gh api -i, read by the same sed on the first line — I kept that, and the code is now in the warning so a run says which refusal it met. An empty status matches no 4?? and falls to *, so a call that reached no response keeps the summary path, which is where the double-post risk lives. 5xx keeps it too.
Your reasoning about 413 is why this is worth doing rather than a widened match for its own sake: the request carries every finding's whole detail, so the size of it is refused as readily as the content, and the summary heading would have told the author GitHub refused their findings when one call each would probably have placed them.
The boundary is pinned from both sides, which matters more than pinning it from one:
- Cases 22, 23 and 24 send
413,403and400. Narrowing the match back to422alone breaks 10 assertions across them — that is the state before this commit. - Cases 5, 15 and 25 send
500and502. Widening the match to[45]??breaks 13 assertions across them, so the retry cannot creep onto the ambiguous outcomes later without a red build.
Not verified: that no 4xx from createReview can be a partial write is HTTP semantics and the endpoint's documentation, not a measurement. No call reached the write API. 413 as GitHub's answer to an oversized body is likewise from the documentation — I did not provoke one.
…ees, and retry on any 4xx
Two answers from the review round on the batched placement call.
A file list from GET /compare is trusted as the diff only when its length
matches the pull request's own changed_files. The endpoint sends at most 300
files and drops the rest in silence: no total, no Link header for them, no
flag. Its pages are pages of commits, and a second page carries no files key,
so --paginate cannot reach the ones it dropped and is gone. Measured against
the live API: a 398-file pull request answers with 300 on one page, while
GET /pulls/{n}/files pages to all 398; a 251-file diff answers with 251.
A short list read as the diff is the one thing the unknown bucket exists to
stop. Every file it dropped looks exactly like a file the pull request never
touched, so each finding in one took a file comment whose body told the author
their cited line was outside a diff that holds it. A short list now indexes
nothing and every finding goes to the API one at a time, which is what base
did for all of them.
The per-finding retry now takes any 4xx, not 422 alone. The request carries
every finding's whole detail, which is model prose under no length bound, so
the size of it is refused as readily as the content and GitHub answers that
with 413. None of 400, 401, 403, 404, 413 or 422 can be a partial write, and
sending them to the summary spent findings that one call each would have
placed. That leaves 5xx and the empty status from a call that reached no
response on the summary path, which is where the double-post risk lives.
The pull request is read once for both fields, so the guard costs no call.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
165 assertions over 29 cases against the shell and jq of `Place findings on the code`, which is the most intricate script in this workflow and had no check on it. run.sh reads the step's run: block and the workflow's own FINDING_MARKER out of the YAML on every run, so a run tests the file as it stands and cannot pass against a stale copy. bin/gh goes on PATH ahead of the real gh: it logs every call, serves fixture JSON through the step's own jq, keeps the request body the step sent, and decides per case whether a call succeeds. No call leaves the runner, so the job needs no token and takes contents: read. The cases cover the hunk walk, the three-way partition, the batch request's shape and marker, the per-finding ladder, the summary headings, the field shapes a model writes, the index commit, the file-count guard, and the status boundary at 4xx against 5xx. Each guard was checked by removing it: dropping the file-count test breaks 6 assertions, narrowing the retry back to 422 alone breaks 10, and widening it to 5xx breaks 13. The fixtures for the generated at-cap case are built by jq at run time rather than committed, so 300 files cost no diff to review. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The harness landed here, and it runs in CIReplying to the non-blocking note in the review body — that one is a review-level bullet with no inline thread, so this is a PR comment rather than an in-thread reply. Committed in b7e9f8d at I took the whole harness rather than the How it runs. Size. 514 lines including fixtures. The 300-file at-cap fixture is built by Each guard was checked by removing it, because a fixture that cannot fail the invariant does not test it:
The harness had already earned this once: after the rebase onto the compare endpoint, the fixtures still modelled
One thing this does not do: it is the first check that runs on this repository's own pull requests, so there was no existing job to hang it on. If a reviewer would rather it landed on its own, say so and I will split it — I kept it here because It has now run. Run 34155941195 on b7e9f8d: Still not verified: the step's own write calls. No call reached the GitHub write API in any of this, and the reviews endpoint has not seen this payload. |
There was a problem hiding this comment.
Batching anchorable findings into a single POST /pulls/{n}/reviews is well-constructed: the commentable-line index is built at the reviewed commit, the short-list guard is a genuinely important safety check, and the 4xx-vs-other split correctly avoids double-posting. No blockers; a few accuracy and degradation issues in the fallback paths are worth tightening.
Findings: 0 blocking | 6 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The new
test/seidroid-reviewharness is a strong addition — extracting the step from the YAML on every run means it cannot pass against a stale copy, and the 25 cases cover the interesting failure modes. Two small gaps: no case exercises a finding whoseseverity/filekey is absent (only empty-string), and no case covers aside: LEFTfinding on a context line, which is the one place the index may be more permissive than the API. run.shprepends$HERE/bintoPATHinsiderun_case, soPATHgrows by one entry per case. Harmless, but hoisting the export out of the function would keep the environment stable across cases.- 4 suggestion(s)/nit(s) flagged inline on specific lines.
| # The caller knows whether the diff carries the line; this does not. | ||
| on_file_or_summary() { | ||
| if [ -n "$head_sha" ] && gh api -X POST "repos/$REPO/pulls/$PR/comments" \ | ||
| -f body="$FINDING_MARKER"$'\n'"**$4** — $5"$'\n\n'"_Cited at \`$1:$2\`, outside this diff's changed lines._" \ |
There was a problem hiding this comment.
[suggestion] When the batch is refused with a 4xx, place_each place_one to_summary_ondiff retries the anchored findings one at a time. If an individual line call then fails, place_one falls through to this file-level comment, whose body asserts _Cited at path:line, outside this diff's changed lines._ — but the index already determined that the line is on the changed lines. Case 4 in the harness exercises exactly this (on_file goes to 3), so an author can receive a file comment that tells them the wrong thing about their own diff.
Since the caller already knows which group the finding came from (it passes to_summary_ondiff vs to_summary as $6), that same signal could select the body: keep the current sentence for the loose group and use something like _Cited at path:line; GitHub would not take a comment on that line._ for the anchored-retry group.
| else 0 end), | ||
| side: ((.side // "RIGHT") | tostring | ascii_upcase | ||
| | if . == "LEFT" then "LEFT" else "RIGHT" end), | ||
| severity: (.severity | tostring |
There was a problem hiding this comment.
[suggestion] .severity | tostring maps a missing or null severity to the literal string "null", not to "note" — the if . == "" then "note" guard only catches an empty string. A finding without a severity therefore reaches the pull request as **null** — <detail>. fx/shift.json uses "severity": "", so the harness does not cover the absent-key case.
(.severity // "" | tostring | if . == "" then "note" else . end) fixes it. The same applies two lines up: .file | tostring turns a missing file into the filename "null", which select(.file != "") does not drop — it survives to burn a file-comment call and then lands in the summary as `null:5`. (.file // "" | tostring) would let the existing select catch it.
| if gh api "repos/$REPO/compare/$base_sha...$head_sha?per_page=100" > "$files.raw" \ | ||
| && jq '[ (.files // [])[] ]' "$files.raw" > "$files" \ | ||
| && n="$(jq 'length' "$files")" && is_count "$n"; then | ||
| if [ -n "$changed_files" ] && [ "$n" -ne "$changed_files" ]; then |
There was a problem hiding this comment.
[suggestion] This equality test will disagree for a common, healthy case: when the head branch has merged the base branch in. GET /pulls/{n} reports changed_files from the merge base of the current base tip and head, while compare/$base_sha...$head_sha resolves the merge base of base.sha and head — and base.sha is not kept in step with the base branch. If head merged base in, merge-base(base.sha, head) == base.sha, so compare returns a superset of the PR's files and n > changed_files.
The consequence is not incorrect placement (the fallback is safe), but such runs lose batching entirely and emit a ::warning:: annotation on an otherwise green job — the noise this step is otherwise careful to avoid. Two options: treat n > changed_files as a non-fatal log line and only refuse on n < changed_files (the truncation direction the guard actually exists for), or read the merge base from compare's own .merge_base_commit.sha and index against that.
| elif ($l | startswith("+")) then .R[.n | tostring] = true | .n += 1 | ||
| elif ($l | startswith("-")) then .L[.o | tostring] = true | .o += 1 | ||
| elif ($l | startswith(" ")) then | ||
| .R[.n | tostring] = true | .L[.o | tostring] = true | .n += 1 | .o += 1 |
There was a problem hiding this comment.
[suggestion] A context line is recorded as commentable on both sides. GitHub documents the opposite for side: "Use LEFT for deletions that appear in red. Use RIGHT for additions that appear in green or unchanged lines that appear in white and are shown for context." So a finding with side: LEFT on a context line is marked anchorable here and, if the API enforces that rule, takes the whole batch down with a 422 — the one failure mode this all-or-nothing call is most exposed to.
The fallback recovers it, so this is a cost rather than a loss, and I have not confirmed that the API actually rejects LEFT-on-context. But the harness only covers LEFT on a genuinely removed line (-gone11), so the case is untested either way. Dropping .L[.o | tostring] = true from this branch (keeping .o += 1) would match the documented rule and cost only the ability to anchor a LEFT comment on unchanged code, which renders identically on RIGHT.
… a step that cannot post (#100) A run cancelled by a newer `@seidroid review` now clears the reactions it left on its own trigger comment, from a step that cannot post one. It left the 👀 there for good. Carries **PLT-1166**. **PLT-1159 comes out ruled out**, on the evidence below. Nobody can do it as written. The permission comment records the reason beside the scope the ticket asked to drop. ## PLT-1166 — the defect `Answer the request` took `!cancelled()`. Two `@seidroid review` comments in quick succession put both runs in one concurrency group under `cancel-in-progress`, so the newer one cancels the older. By then the older comment already wears the 👀 — the acknowledgement is the first step of the job. The newer run answers its **own** comment id, so nothing ever reads the older one again. The result: a comment that asked for a review, wears eyes, and never gets an answer. That is the defect PLT-1144 fixed on the no-verdict path, reached by the one path that fix does not cover. ## What ships `Answer the request` keeps `!cancelled()` — unchanged from the base. The withdrawal is a new step, last in the job: ```yaml - name: Withdraw the reactions on a cancelled run if: ${{ inputs.mode == 'review' && cancelled() && needs.guard.outputs.comment_id != '' && steps.verdict.outputs.posted != 'true' }} ``` `Post the verdict` gains `id: verdict` and a `posted` output so that last term can read it, and `Answer the request` gains `id: answer`. Nothing else in the workflow changes. ### Why not `always()` on `Answer the request` That was the first shape here and it was wrong. A step output persists once its step completes, so a cancellation landing any time after `drive` finishes leaves `check_path` and `verdict_produced` populated. `Answer the request` would read a real conclusion and post a thumb, while every publisher skips on `!cancelled()`. A thumb reads as an answer. That is worse than the stale eyes this PR set out to remove. ### Why the new step cannot state an outcome **It contains no POST.** The script lists this bot's reactions and deletes the three this workflow posts. No code path in it adds one. Nothing the step receives can therefore make it state an outcome. That property holds whatever its inputs are, which is what makes it structural rather than a matter of what a cancellation happens to look like. It reads no check file, no `verdict_produced` and no conclusion. ### What the `steps.verdict.outputs.posted` term is, and why it does not break that A cancellation can arrive once the verdict is already on the pull request — during thread resolution, say — and the thumb `Answer the request` posted answers it correctly. Withdrawing it there leaves a published review with no reaction on the request that asked for it, which reads as never answered. That is this step's own defect, one window later. The posting step's **`posted` output** separates the two. It is one boolean about another step, written from the comment POST's own result. No conclusion is in it. It says whether an answer already stands, never which answer it would be, so reading it gives the step nothing to state. Handing it `verdict_produced` instead would have restored the conditional reasoning above: that flag is true whenever the driver reached a verdict, including when nothing published. **Its outcome will not do, and that took a second pass to see.** `Post the verdict` runs under `continue-on-error` and tolerates a refused comment POST. Its failure path ends on a call whose failure it swallows. The step therefore exits 0, and its outcome reads `success` whether the verdict landed or not. The first version of this gate read that outcome. A refused POST followed by a cancellation then kept a thumb standing for a review nobody can see. That step already tracked the POST's result in a shell variable. It now writes it as an output. Anything but a posted verdict withdraws. A value the step cannot read therefore clears. It does not leave a thumb standing for a verdict that may not be on the pull request. ### The cross-run half of the same problem A re-run replays the trigger comment id. The comment can therefore already carry a thumb from an **earlier** run whose verdict is on the pull request. A re-run cancelled before it answered took that thumb along with its own eyes. The comment then ended bare while the verdict it asked for still stood. `Answer the request` gains `id: answer`, and the withdrawal reads its outcome to decide what this run may take: | `steps.answer.outcome` | what it means | withdrawn | |---|---|---| | `skipped` | this run never touched the comment, so a thumb there is an earlier run's | `eyes` only | | `success` | this run withdrew the stale thumb and posted its own, and published nothing | `+1 -1 eyes` | | `failure`, `cancelled`, unreadable | the step ran partway and most likely took the earlier thumb already | `+1 -1 eyes` | The structural property is untouched: still no POST, and an outcome is still four words about another step with no conclusion among them. **One case survives, and the comment states it rather than claiming it away.** A run answers, which withdraws an earlier thumb and posts its own. A cancellation then arrives before publishing, and the comment ends bare. The answer step already took the earlier thumb, so nothing at the end of the job can put it back. Knowing it happened would need a read of the pull request this step deliberately does not make. A cancellation lands during the driver far more often than in that gap. The step's comment records the limit instead of asserting the invariant outright. **One tension with the stated acceptance criterion, deliberately.** "Given a run cancelled by a newer request, its trigger comment carries no reaction from this bot" now fails on one path. A cancelled re-run leaves an earlier run's thumb. The criterion's intent holds. The comment does not wear 👀 with no answer coming, because the answer is on the pull request. Satisfying the literal wording would restore the defect above. I flag it rather than read the criterion loosely. ### Why last in the job, and what holds it there The runner evaluates a step's condition when it reaches the step. **Any step after the withdrawal is a step during which a cancellation leaves the eyes standing.** The runner already evaluated the withdrawal and skipped it by then. Placed last it also reads `steps.verdict.outputs.posted` after that step has reported. `conditions.py` checks the position rather than any one ordering, which covers a step appended later. Four mutations fail it. Move the withdrawal ahead of `Post the verdict`, ahead of the resolve step, or ahead of the no-verdict report. Or append a step after it. The id-ordering check caught only the first. In the other three `Post the verdict` still ran earlier. ### Why not the fix as named Gating the conclusion read inside the script needs the job status in the shell, and GitHub does not offer it there. `cancelled()` is readable only in a step or job `if`. `PipelineTemplateEvaluator.EvaluateStepEnvironment` calls `CreateContext(contextData, expressionFunctions)` with no `expressionState`, where `EvaluateStepIf` passes `step.ExecutionContext.ToExpressionState()`. And `CancelledFunction.EvaluateCore` reads `templateContext.State[nameof(IExecutionContext)]` and `ArgUtil.NotNull`s it. `StepsRunner` turns that throw into `CompleteStep(step, TaskResult.Failed)`, so `env: CANCELLED: ${{ cancelled() }}` fails the step on every run, before the runner evaluates its condition. actionlint refuses it too: `calling function "cancelled" is not allowed here. "cancelled" is only available in "jobs.<job_id>.if", "jobs.<job_id>.steps.if"`. The same holds for `run:`. ## The cancellation shapes, and which this covers | when the cancellation lands | what runs | outcome | covered | |---|---|---|---| | while queued, job never starts | nothing | no eyes were ever posted | n/a | | before `drive` completes | withdrawal | all three withdrawn, no thumb | yes | | **after `drive` completes** | withdrawal | **it cannot read the populated outputs** | yes | | while `Answer the request` runs | its `!cancelled()` re-test fires, the runner kills it, then the withdrawal | the withdrawal takes whatever it left | yes | | after the thumb, before the verdict published | answer, then withdrawal | the withdrawal takes the thumb: it would stand for nothing | yes | | **after the verdict published** | answer only | **thumb survives beside the published verdict** | yes | | **the verdict POST refused, then cancelled** | answer, then withdrawal | **thumb withdrawn: its outcome still reads success** | yes | | during the withdrawal step | withdrawal | its own condition is `cancelled()`, so the re-test keeps it alive | yes | | once the runner reached every step | answer only | a thumb this run earned stays | correct | | a re-run, cancelled before it answered | withdrawal | an earlier run's thumb stays, its eyes go | yes | | a re-run, answered then cancelled before publishing | answer, then withdrawal | bare comment, earlier verdict stands | **no** | | **runner process shutdown** (`RunnerShutdownToken`) | nothing | `StepsRunner` skips condition evaluation outright | **no** | Two rows are gaps. In the answered-then-cancelled row the answer step has already taken the thumb, so no later step can restore it. A hard kill of the runner leaves the eyes on the comment, and nothing inside a workflow closes that. ## The reaction table Each case declares two job states: the one the runner reached `Answer the request` in, and the one it reached the withdrawal step in. `success>cancelled` is a cancellation that arrived after the answer, so the answer step posts its own thumb and the fixture places nothing by hand. | case | states | verdict outcome | ran | left on the comment | |---|---|---|---|---| | success | `success>success` | success | answer | `bot:+1` | | failure | `success>success` | success | answer | `bot:-1` | | no verdict | `success>success` | skipped | answer | *none* | | neutral | `success>success` | skipped | answer | *none* | | **cancelled after `drive`, outputs populated** | `cancelled>cancelled` | skipped | withdraw | ***none*** | | cancelled before `drive` finished | `cancelled>cancelled` | skipped | withdraw | *none* | | cancelled mid-publish | `success>cancelled` | cancelled | answer+withdraw | *none* | | the verdict failed to post | `success>cancelled` | failure | answer+withdraw | *none* | | the outcome went unreported | `success>cancelled` | *empty* | answer+withdraw | *none* | | **cancelled after the verdict published** | `success>cancelled` | success | answer | ***`bot:+1`*** | | the same, beside a human's | `success>cancelled` | success | answer | `brandon:-1`, `bot:+1` | | success, human `+1 -1 eyes` | `success>success` | success | answer | human ×3, `bot:+1` | | failure, human ×3 | `success>success` | success | answer | human ×3, `bot:-1` | | no verdict, human ×3 | `success>success` | skipped | answer | human ×3 | | cancelled, human ×3 | `cancelled>cancelled` | skipped | withdraw | human ×3 | | stale `bot:-1` + `human:+1` | `success>success` | success | answer | `human:+1`, `bot:+1` | | the same, cancelled | `cancelled>cancelled` | skipped | withdraw | `human:+1` | | stale `bot:+1`, no verdict | `success>success` | skipped | answer | *none* | | `bot:rocket` from another workflow | `success>success` | success | answer | `human:+1`, `bot:+1`, `bot:rocket` | | the same, cancelled | `cancelled>cancelled` | skipped | withdraw | `human:+1`, `bot:rocket` | | the list call refused | `success>success` | success | answer | `human:+1`, `bot:+1`, `bot:eyes` + warning | | a delete refused | `success>success` | success | answer | `bot:+1`, `bot:eyes` + warning | | the add refused | `success>success` | success | answer | *none* + warning | | the list refused, cancelled | `cancelled>cancelled` | skipped | withdraw | `bot:eyes` + warning | | a delete refused, cancelled | `cancelled>cancelled` | skipped | withdraw | `bot:eyes` + warning | | the acknowledgement refused | `success>success` | success | answer | `bot:+1` | A human's reaction survives every path. A `bot:rocket` some other workflow left survives too, because each step deletes only the contents this workflow posts. And a thumb that answers a published verdict survives a later cancellation. ## PLT-1159 — ruled out, with the evidence The ticket's premise is that `ai-review.yml` reaches the same reactions through GraphQL `addReaction` under `pull-requests: write`. **It does not, and the workflow file does not decide the question.** **1. `ai-review.yml` does call both mutations, and its `permissions:` blocks do lack `issues`.** `preflight` is `contents: read` + `pull-requests: write` and calls `addReaction(content: EYES)`; `complete_review_reaction` is `pull-requests: write` alone and calls `addReaction(THUMBS_UP)` then `removeReaction(EYES)`. **2. But neither call uses `GITHUB_TOKEN`.** Both steps pass `github-token: ${{ steps.app-token.outputs.token || github.token }}`, and in production the App token wins. I checked live comments. Every reaction on an `@seidroid review` trigger in `sei-chain` belongs to `seidroid[bot]`, not to `github-actions[bot]`: ``` comment 5536974191 +1 by seidroid[bot] sei-chain#4088 comment 5544795940 +1 by seidroid[bot] sei-chain#4101 comment 5531689281 +1 by seidroid[bot] sei-chain#4095 comment 5493838638 +1 by seidroid[bot] sei-chain#4063 ``` The workflow's `permissions:` block does not bound an App installation token. Its own installation grant governs, and that App holds Issues: write — `ai-assistant.yml` posts `POST /repos/{o}/{r}/issues/comments/{id}/reactions` with the same token. ai-review is therefore **no evidence at all** about what `pull-requests: write` alone can do. It is the same class of wrong premise as the `enable-cursor: false` one. **3. GitHub documents no permission for any GraphQL mutation.** Not a gap in my reading — checked at the data source. In `github/docs`, `src/graphql/data/fpt/schema-reactions.json` gives `addReaction` and `removeReaction` the keys `name, id, href, description, isDeprecated, inputFields, returnFields, category` and no permission field. The public SDL (`docs.github.com/public/fpt/schema.docs.graphql`) carries only `@docsCategory(name: "reactions")`. The GraphQL guide's whole statement on the subject is that the API returns an error naming the permission it wanted. One route therefore remains to the requirement: make the call. **4. GitHub documents what REST requires, and the alias stops at the reaction.** `github/docs`, `src/github-apps/data/fpt-2026-03-10/server-to-server-permissions.json`: | endpoint | permission | |---|---| | `GET/PATCH/DELETE /repos/{o}/{r}/issues/comments/{id}` | listed under **both** `issues` and `pull_requests` | | `POST /repos/{o}/{r}/issues/comments/{id}/reactions` | `issues: write` **only** | | `DELETE /repos/{o}/{r}/issues/comments/{id}/reactions/{rid}` | `issues: write` **only** | | `POST /repos/{o}/{r}/pulls/comments/{id}/reactions` | `pull_requests: write` (a *review* comment — a different resource) | That file expresses "either permission" by listing an endpoint twice. A single listing on the reactions endpoints is therefore a distinction, not an omission. It confirms the claim the guard job already makes in prose. **Verdict.** The premise is void and the documentation says nothing. Nothing here can mint a fine-grained token scoped to `pull-requests` to test it. Shipping the drop blind would regress the defect this PR fixes. `continue-on-error` and a `::warning::` swallow a 403 on the reaction, so the eyes would stay on every comment and no run would fail. Ruled out. ### Two things worth keeping for whoever re-files it **`removeReaction` beats the REST loop, whatever the scope turns out to be.** The ticket assumed it takes a reaction node id. It does not. `RemoveReactionInput` is `{content: ReactionContent!, subjectId: ID!}`, and the subject is the *comment*: `IssueComment` sits in its `@possibleTypes`. It takes no actor input, so it can only ever remove the viewer's own reaction. That makes the human-scoping property structural rather than a `select(.user.login == $me)` filter. It also retires the hardcoded `me="github-actions[bot]"` login and the paginated list whose miss leaves eyes behind. **A third shape the ticket does not name looks likelier than either.** This workflow already mints an App token (`steps.identity.outputs.token`) and already computes `REVIEWER_LOGIN` from `app-slug`. Reacting under that identity needs no caller scope at all, and it is how production already posts these reactions. Two obstacles stand in the way. The acknowledgement runs before the mint, on purpose. And the mint is optional, so a `GITHUB_TOKEN` fallback keeps the scope required — unless a caller without App credentials may lose the reaction. ## The cost this change carries The list-and-delete block is now duplicated between `Answer the request` and the withdrawal step. `me="github-actions[bot]"` and the set of contents each step may delete are two copies, and a reader has to keep them in step by hand. Edit one and not the other and a reaction stays behind on whichever path lost the edit. That is the price of the separate step, and it buys the structural property: the withdrawing step has no POST. Sharing the block would mean one step doing both jobs, which is the shape that produced this PR's blocker. GitHub Actions offers no way to share a script between two steps without a checkout, and YAML anchors are not supported. Both harnesses cover both copies, so a drift fails rather than ships. The GraphQL `removeReaction` above is what would remove the duplication outright. It needs no login and no listing, so the whole block collapses to one mutation per content. ## Every reaction site Three steps, six calls, all in the `review` job, all on the ISSUE comments endpoint: | line | step | call | |---|---|---| | 1036 | `Acknowledge the trigger` | `POST .../issues/comments/{id}/reactions` (`eyes`) | | 2494 | `Answer the request` | `GET .../reactions --paginate` | | 2504 | `Answer the request` | `DELETE .../reactions/{rid}` | | 2519 | `Answer the request` | `POST .../reactions` (`+1` / `-1`) | | 3292 | `Withdraw the reactions on a cancelled run` | `GET .../reactions --paginate` | | 3304 | `Withdraw the reactions on a cancelled run` | `DELETE .../reactions/{rid}` | The withdrawal step has no `POST`, and that is the fix. `guard` reacts nowhere. `ai-assistant.yml` and `ai-review.yml` have their own sites; neither is in this workflow. ## Verification **Committed, not kept locally.** An uncommitted harness is how the first blocker survived a reading and seven mutations. Two additions to `test/seidroid-review/`, wired into `workflow-test-self.yml` as their own job so the placement check keeps its name. **`reactions.sh` — 62 assertions over 32 cases.** It runs the reaction steps under `bash`, extracted from the workflow on every run. No case names the step it runs. `conditions.py --select` names it, from the job state and the posting step's outcome, so the two layers cannot drift. The `gh` stub keeps the reaction list a comment carries and serves it through the step's own `--jq`. It honours idempotence per (user, content). It can refuse the list, a delete or the add. The acknowledgement's calls go to a separate log, so every count belongs to the step under test. The stub reports any call it cannot serve. **`conditions.py` — 76 assertions.** A step condition decides which reaction step runs in which job state, and a shell harness cannot see it. The model applies the runner's own rule: a condition naming none of `always`/`cancelled`/`failure`/`success` becomes `success() && (...)`. It treats a term it cannot decide as unknown rather than false. Two checks read the file rather than a table, so they cover a step added later: Both walk **every job's raw steps list** and search the **whole step**: - No step that can run on a cancelled job may reach `check_path` or `verdict_produced`. - Every `steps.<id>` a step reads must be a real id on an earlier step. - The withdrawal is the last step of the review job. Keyed off a name they dropped an unnamed step. `- uses: ...` with no `name:` is the usual shape, so the step most likely to arrive later was the one they could not see. The guard job already carries one. The id check also reached only `if`, which left the withdrawal's new `env` read unchecked. **Each fixture, mutation-tested.** A fixture that cannot fail the invariant is not a test of it: | mutation | which case fails | |---|---| | **the gate reads `steps.verdict.outcome` again** | **`THUMB GOES` on a refused POST, and the matching condition row** | | **the `skipped` arm takes all three** | **`EARLIER THUMB SURVIVES`, plus 3 more** | | **`id: answer` deleted** | **`reads steps.answer, which is no step's id`** | | **an unnamed `always()` step reading `verdict_produced`** | **`step 17 runs on a cancelled run and reads verdict_produced`** | | the `posted` term dropped | `THUMB SURVIVES` + 3 more + the condition row | | the same term inverted | 16 script cases, 5 condition cases | | `id: verdict` deleted | `reads steps.verdict, which is no step's id` | | the withdrawal moved ahead of `Post the verdict` | the position check, and `reads steps.verdict, which runs later` | | **the withdrawal moved ahead of the resolve step** | **the position check alone — the id check passes it** | | **the withdrawal moved ahead of the no-verdict report** | **the position check alone** | | **any step appended after the withdrawal** | **the position check** | | `Answer the request` back to `always()` | its cancelled row, and the sweep | | `check_path` interpolated inline into `run:` | the sweep — an `env`-only sweep passes it | | the withdrawal step deleted | `no step named 'Withdraw the reactions on a cancelled run'` | | the withdrawal not scoped to this bot | the human and `rocket` cancelled cases | | the withdrawal ignoring which contents it may take | `foreign-cancelled` loses a `rocket` | | the missing-conclusion arm clearing only the eyes | cancelled-with-stale-thumb, no-verdict-with-stale-thumb | | the answer's list not scoped to this bot | every human case | | the empty-reaction guard removed | every clear-only case posts a blank reaction | | the `VERDICT_PRODUCED` gate removed | no verdict thumbs the requester down | | the success arm no longer naming the eyes | eyes survive a green review | Only `conditions.py` catches `id: answer` deleted. `reactions.sh` derives that outcome itself, which is exactly why the id check has to exist. `conditions.py` also models `steps.verdict.outcome` beside `posted`, derived rather than passed. It reads `success` whenever that step reported at all, which is what the runner sees. A gate that regresses to the outcome therefore fails an assertion instead of crashing the model. **Five checks ran narrower than their own claim.** Mutating the thing each claimed to cover is what found them. - The haystack read `env` only, so an inline interpolation passed. - The model took an outcome as an argument and never checked the id existed. - The sweep keyed off a step name, so an unnamed step was invisible. - `run_case` exported each per-case knob without clearing it. An `ANSWERED_AS` override leaked forward and disarmed a later case. - The gate read an exit code that cannot express whether the verdict landed. The first three now read the file. `run_case` clears the fourth at the top of every case. The fifth reads an output written from the POST's own result. **actionlint**, base vs branch, both rule sets. `seidroid-review.yml`: 4 × `SC2102:info`, unchanged. Whole `.github/workflows`: 37 findings, identical after line-number normalisation. `workflow-test-self.yml`: 0. `shellcheck -S warning` clean on both new harness files. The incumbent harness still passes, now 271 assertions after #102. **Base checked three times, and no move trusted.** It went to `b1b51f8` (#97), then `2f58b80` (#101), then `98c2619` (#102). #97 rewrote 341 lines of this file and added the harness. #102 rewrote the resolve step and placement, and renamed the harness job. Each time all three harnesses and actionlint ran again on the new history. None of them carried a result from before it. I checked each rebase by reading back four things: the step order, the step ids, the gate, and the withdrawal step's POST count. Not by its exit status. The #102 rebase conflicted once, in the harness README, and I resolved it keeping both sides. `workflow-test-self.yml` merged cleanly: `place-findings` keeps #102's renamed display name `Place findings and resolve threads`, and the reaction job keeps its own. **One handoff for whoever lands second.** #103 replaces the hardcoded `issues/comments` with a `comment_api` output. Its author wrote it against **two** reaction steps. This branch leaves three, carrying six hardcoded paths rather than four: one in `Acknowledge the trigger`, three in `Answer the request`, two in the withdrawal step. Whichever of us rebases second has to reach all three steps. ## Not verified **Nothing here ran on a GitHub runner.** Five things stay unverified. That a real cancelled run reaches the last step of the job. That `steps.verdict.outcome` reads `success` on a run cancelled after that step completed. That `steps.answer.outcome` reads `skipped` rather than empty on a run cancelled before that step. That the runner's condition re-test kills `Answer the request` mid-flight, as its source says. And every permission claim above. The cancellation semantics rest on `actions/runner` source and on actionlint. `conditions.py` models the expression engine; it is not the engine. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Findings the diff can hold now travel in one
POST /pulls/{pr}/reviewscall carrying the review body and every inline comment together. An author with ten findings gets one review and one notification instead of ten ungrouped threads, and the run spends one call where it spent ten.What changed
Place findings on the codeposted onePOST /pulls/{pr}/commentsper finding and let the API decide the rung by refusing. It now reads the pull request's own diff first, splits the findings against it, and posts the anchorable ones as a single review.The three rungs hold, and the middle one still costs a call of its own: the reviews API carries
path,line,sideandbodyper comment and has nosubject_type, so a file-level comment cannot ride in the batch.POST .../comments, one per findingPOST .../reviewsfor all of themPOST .../commentswithsubject_type=file$NOTEHow
ai-review.ymldoes it, and what transferred.github/workflows/ai-review.yml:860-880walks eachpulls.listFilespatch into a set of commentable lines per file and side. RIGHT takes an added or a context line, LEFT takes a removed or a context one. Transferred, as a jq walk over the same patches. It is what makes the batch safe to send.ai-review.yml:897,901anchors a finding whose line is in that set and orphans the rest into the body. Transferred in shape: an unanchorable finding here takes the file rung first and the summary second, which is this workflow's own ladder and is richer than orphaning.ai-review.yml:960-967builds thecommentsarray and onecreateReviewcall carryingcommit_id,event,bodyandcomments. Transferred.ai-review.yml:968-991falls back inline → body-only → COMMENT. Not transferred as written. That chain protects a review body this step does not own: the verdict and the summary go out as an issue comment fromPost the verdict, and the position goes out fromState the review's position. The fallback here is different and is answer 4 below.The four points
1. The finding marker. Every inline comment in the batch opens with
FINDING_MARKERas its first bytes, built in jq as"\($marker)\n**\(.severity)** — \(.detail)". The review's own body carries no marker. The history read and the resolve step both key on(.comments.nodes[0].body) | startswith($ENV.FINDING_MARKER)over review threads. A comment sent in acreateReviewcall opens a thread whose first comment is that comment, which is measured rather than assumed: the same GraphQL query those two steps run, against #90, returns five threads whosecomments.nodes[0]is agithub-actionscomment belonging to anAPPROVEDreview —ai-review.yml's own batch. A marker on the review body would be read by nothing and is left off. Case 1 in the table below asserts the marker is the first bytes of all four comment bodies; case 3 asserts the same for the seven bodies the per-finding fallback sends.2.
unplacedon the merge gate. What counts as placed is unchanged.on_linestill counts findings the API accepted, and it is incremented only after the review call returns success.on_fileandunplacedare untouched.Resolve the threads this review closedreadsplaced = on_line + on_fileand requiresunplaced == 0; the harness shows base and branch agreeing on all three counts in every case where the API cooperates.One case moves, and it moves toward holding the gate: when the batch fails with a 5xx or with no status at all, all of its findings go to the summary, so
unplaced > 0and no superseded thread closes. Base would have placed those findings one at a time.3. Riding on the position step's call. Rejected; this is a second review object. Four reasons.
State the review's position on the pull requestrecords no review at all on asuccessconclusion withapprove-on-successfalse, or onneutral. Those runs have findings and no call to ride on.steps.place.outputs.*. Posting there would make placement report intent rather than outcome, and the resolve gate would close threads on the strength of comments that had not been sent.continue-on-error, because it holds the only withdrawal of a standing block. An all-or-nothing comment batch on that call lets one bad line cost the position and the withdrawal.GET /pulls/{n}/reviewson fix(seidroid-review): fall back to GITHUB_TOKEN for the position and the dismissal #88, fix(seidroid-review): refuse an explicit re-review on a fork-originated pull request #89 and feat(seidroid-review): resolve the threads a re-review closed #90 returns oneCOMMENTEDreview with"body": ""per standalone comment. Ten findings are ten review objects today and one after this.The ticket says the position step is skipped on a no-verdict run while placement is not. That is not true on this base: both gate on
inputs.mode == 'review' && !cancelled() && steps.drive.outputs.verdict_produced == 'true', character for character. The real asymmetry is the empty-eventpath above.4. The batch is all-or-nothing. Two failures, two answers.
4xx. The API refused the request and created nothing. Each finding is posted on its own down the full three-rung ladder, so the one comment that was refused costs only itself. This request carries every finding's wholedetail, which is model prose under no length bound, so its size is refused as readily as its content and GitHub answers that with413. Cases 3, 4 and 22 to 24 cover422,413,403and400; case 4 refuses the batch and then one line individually, and that finding lands on its file while the other three land on their lines.unplaced > 0holds the superseded threads open. Cases 5, 6 and 25.The code is read from the response's own status line, which
gh api -iputs first — not from astatusfield in the error body. GitHub's validation-error schema does not declare that field, and a refusal carrying none would have read as no refusal at all and sent every anchorable finding to the summary, which is worse than base. Measured on this endpoint: a 422 does carrystatustoday, and the schema does not promise it.A refusal is predicted rather than met: the commentable-line index is built before the call, from the diff at the reviewed commit.
GET /pulls/{n}/filesanswers for the pull request's current head and takes no commit — measured, it accepts ashaparameter and ignores it, so a push mid-review would index one commit and comment on another.GET /compare/{base.sha}...{REVIEWED_SHA}does take one; measured on #90, it answers differently per commit and reproducespulls/{n}/filesbyte for byte at the head. Its three-dot form is the diff the pull request shows, and a base branch that moves during the review does not move that merge base, because the reviewed commit is fixed.The index is read as the diff only when its length matches the pull request's own
changed_files, which the sameGET /pulls/{n}call already answers.GET /comparesends at most 300 files and drops the rest in silence: no total, noLinkheader for them, no flag. Its pages are pages of commits, and a second page carries nofileskey at all, so--paginatecannot reach the ones it dropped; it is gone from the fetch, which now costs one call instead of one per hundred commits.Measured against the live API.
kubernetes/kubernetes#137092reportschanged_files: 398;GET /compare/{base}...{head}answers with exactly 300 on a single page, whileGET /pulls/{n}/files --paginatereturns all 398. Three-dot ranges of 309 and 321 files both answer with 300, and one of 251 answers with 251.changed_filesand the compare length agree on every whole list measured, including 295 and 261 — just under the cap.A short list read as the diff is the one thing the
unknownbucket exists to stop: every file it dropped looks exactly like a file the pull request never touched, so each finding in one took a file comment whose body told the author their cited line was outside a diff that holds it. A short list now indexes nothing. Cases 18 to 21 cover a short list, a list at the cap the count confirms, a list at the cap with no count to confirm it, and a list under the cap with no count.When the index cannot be built — the base commit or the diff cannot be read — the step falls back to posting each finding on its own, which is what it did before. Cases 10 and 14c.
A file the API sends without a patch is a third group, not a file with no lines. A binary file and a file whose diff was too large both arrive that way, so counting them as empty would drop every finding on one to the file rung under a body claiming the cited line is outside the diff. Those findings go to the API one at a time instead. Case 17.
Verification
Nothing here ran on a GitHub runner. The step's script was extracted from the shipped YAML with a YAML parser and run under
bashwith aghstub onPATHthat serves fixture JSON through the step's ownjqand captures the request body.baseis the same harness against the same step extracted frombc93b4f.Columns: API calls made, then the three counts written to
$GITHUB_OUTPUT, then lines in the summary note.REVIEWED_SHAstatuschanged_filesCases 14 and 14b read
pullandcomparecalls too; 14 asserts the compare range is{base.sha}...{REVIEWED_SHA}.Case 18 is the regression this round fixes:
pkg/b.gois in the pull request and missing from the compare response, which is what truncation looks like. Read as the diff it puts the finding onpkg/b.gounder a body saying line 2 is outside a diff that adds line 2. Case 19 is the case that must not bail — a genuine 300-file pull request whose count agrees — so the guard cannot simply refuse any list of 300.Every case exits 0.
continue-on-error: trueis unchanged on the step.Base and branch produce identical counts in cases 1, 2, 3, 4, 7, 8, 9, 10, 11 and 14c. They differ in 5, 6, 15 and 15b by design (answer 4), in 14 and 14b because base has no index at all, and in 12, 13 and 17 for the reasons below.
165 assertions over 29 cases pass, including: the request carries
event: "COMMENT", the recordedcommit_id, a non-empty body, four comments in findings order with the rightpath/line/side, each opening with the marker as its first bytes; a multi-line detail and one carrying a backtick, a double quote and a$survive intact; the review body starts with neither marker.Also verified:
actionlintfinds the same fourSC2102:infoat the same in-script offsets before and after, and the whole-repo output is identical apart from line numbers.shellcheck -S styleon the extracted script is clean. The file parses as YAML. The harness now lives attest/seidroid-review/and runs in CI from.github/workflows/workflow-test-self.yml; it re-reads the step and the marker out of the YAML on every run, so it cannot pass against a stale copy.Each guard was checked by removing it. Dropping the file-count test breaks 6 assertions, narrowing the retry back to
422alone breaks 10, and widening it to 5xx breaks 13.Not verified: nothing ran on a GitHub runner, and no call reached the GitHub write API. The atomicity of
createReview, the claim that no 4xx can be a partial write, the 413 answer to an oversized body, andgh's placement of the error object on stdout are taken from the API documentation and from HTTP semantics, not measured. The read side — the 300-file cap, the absentfileskey on page two, andchanged_files— is measured against the live API as above. Nothing here proves GitHub accepts this exact payload; the first real run does.The harness also caught two defects in this branch before it shipped. The fixtures still modelled
pulls/{n}/files, so every case fell to the ladder and 46 assertions failed loudly rather than passing quietly. Andgh api --jqwrites the error object itself on a failure, so a refused base read left a line of JSON inbase_sha; it is now tested for the shape of a commit id, not merely for emptiness.Two things the harness found in the base
Both are in the record the fallback rungs read, and this change rewrites that reader, so they are fixed here rather than left behind.
@tsvwrites an empty field as nothing between two tabs, andIFS=$'\t' readfolds the pair into one delimiter because a tab is IFS whitespace. A finding with nosidetherefore read one field short: the severity arrived in the side, the base64 detail arrived in the severity, and the detail was lost. On base, a finding with an empty severity renders in the summary as- `pkg/untouched.go:3` () —with no text at all. The normalising jq now guarantees every field but the last is non-empty.ghas a field. Base passes-F line="$line"straight from the findings file, andghreads a leading@as a file to send. A finding whose line is@/etc/passwdproduced exactly that call. Lines are now coerced to an integer, and a line that is not one reads as 0, which no diff covers, so the finding takes the file rung.Scope
The 50-finding cap is not in this workflow; the driver writes
findings.jsonand bounds it. Thread resolution, the cap, and the driver and caller repositories are untouched.Residual: a diff so large the API sends no patch for a file a finding names. That finding costs one line call before the API answers, which is what base cost for every finding. A file present without a patch is handled as unknown, and a file list short of the pull request's own count now indexes nothing.