Conversation
…sable workflow seidroid-review.yml reviews a pull request from inside the Actions runner and publishes what it found: inline comments on the changed lines, a check run, and a review position. A caller wires it with `uses:` and passes its own secrets. This folds 33 commits from feat/seidroid-review into one. `main` carried none of this work, so the whole change is an addition. What the workflow does: - Reviews once per pull request automatically, and again whenever a person asks by name. The guard decides both, and refuses a fork-originated re-review. - Places every finding the diff can hold in ONE review, so the author reads the verdict and the lines it cites on a single notification. A finding the diff cannot anchor degrades to its file, then to the summary. It is never dropped. - Carries the verdict in that same review (PLT-1268). A standalone comment is the fallback, for a run where no review landed. - Closes a thread only once its own replacement reached the code, so a re-review does not leave the author reading one finding in three places. - Withdraws a block it left earlier when a later review no longer reproduces it. - Acknowledges the trigger with a reaction, and withdraws it on a cancelled run from a step a cancelled run can still post from. The driver floor is v0.21.0, in the `driver-version` default and in MIN_DRIVER_VERSION. Both name one release on purpose: the conclusion a review reaches for a given set of findings is specific to the driver that reached it, and v0.21.0 is the first that renders the verdict from the structured block. Tests: seven harnesses, 739 assertions. Each reads the steps out of the YAML on every run, so none can pass against a stale copy. workflow-test-self.yml runs all seven. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR SummaryMedium Risk Overview PLT-1268 behavior: the driver-rendered verdict is the body of the single COMMENTED review that holds inline findings when placement succeeds; Also adds admission rules (fork/draft/label/re-review), supersede-and-resolve thread handling, withdrawal of stale CHANGES_REQUESTED blocks, trigger reactions, and Reviewed by Cursor Bugbot for commit f841f0c. Bugbot is set up for automated code reviews on this repo. Configure here. |
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 f841f0c. Configure here.
| echo "::warning::could not append the findings line and the unplaced observations to review $REVIEW_ID on $REPO#$PR; the verdict that review carries still stands, and what did not reach it follows" | ||
| echo "--- what the review does not carry ---" | ||
| printf '%s\n' "$body" | ||
| echo "--- end ---" |
There was a problem hiding this comment.
Submitted review body cannot update
Medium Severity
Post the verdict updates the placement review with PUT /reviews/{id} after Place findings already submitted that review as event: COMMENT. GitHub accepts that update only on a pending review, so the findings line and the unplaced-observations note never reach the pull request. Every successful batch placement hits the warning path instead.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit f841f0c. Configure here.
There was a problem hiding this comment.
A large, carefully documented and well-tested addition of the seidroid reusable review workflow, but the base-standards fetch issues a POST instead of a GET, so the repository's accepted-conditions list is never read and the failure is silently misreported as an absent file. Everything else is non-blocking.
Findings: 1 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) listed below under unanchored comments.
Non-blocking
Read the base branch's accepted conditionsis the one step in the review job with no coverage intest/seidroid-review/— the harness covers placement, resolve, reactions, guard admission, decision, verdict destination and the deadlines, but nothing exercises this fetch or its 404/empty/error branches. A stub case asserting that the request is a GET and thatpath/refare emitted for a file that exists would have caught the POST bug above.Post the verdictandReport a review that reached no verdictboth locate the stale no-verdict notice with... | tail -n 1, so if an earlier deletion failed only the newest duplicate is ever removed and older copies persist for the life of the pull request. Deleting every matched id would make the "one notice per pull request" invariant hold on its own.- 1 suggestion(s)/nit(s) flagged inline on specific lines.
Comments that couldn't be anchored to the diff
.github/workflows/seidroid-review.yml:1893-- [blocker]-f ref="$base"with no--method/-Xmakes this a POST, not a GET.gh apidefaults to GET only while no parameters are supplied; the first-fflips it to POST and sends the params as a JSON body. This file already states that rule itself at line 1126 ("-Fon agh apithat names no-Xmakes the request a POST"), and every other read here keeps its params in the path for exactly that reason — this is the only one that doesn't.
POST /repos/{owner}/{repo}/contents/{path} is not a routed method, so the call fails with HTTP 404: Not Found even when the file exists on the base branch. That message then matches the *"HTTP 404"*|*"Not Found"* arm below and logs "$REPO has no $file on $base; nothing is accepted" — so the failure is misreported as the routine absent-file case, the warning that was written to catch a 403 or a rate limit never fires, and path/ref are never set. --base-standards-file is therefore never passed to the driver on any repository, and every pre-existing blocker the base branch had accepted silently withholds approval again.
Fix:
if err="$(gh api --method GET "repos/$REPO/contents/$file" -f ref="$base" \
-H "Accept: application/vnd.github.raw" 2>&1 > "$out")"; thenThe -f ref= form is still the right choice for the encoding reason the comment above gives — it just needs the explicit method to stay a GET.
.github/workflows/seidroid-review.yml:2953-- [suggestion] The dismissal selector matches onstate == "CHANGES_REQUESTED"and the marker alone, with no test on who wrote the review. A human review that opens with this file'sVERDICT_MARKER— a maintainer quoting the bot's verdict at the top of their own "Request changes" — would be dismissed by the next clean run, clearing a blocking human review that this workflow has no standing to withdraw.
The guard already applies the narrower test for the same question at line 1141 (select(.user.type == "Bot" and ...)), so adding it here just makes the two agree:
[.[] | select(.user.type == "Bot" and .state == "CHANGES_REQUESTED" and ((.body // "") | startswith($ENV.VERDICT_MARKER)))] | .[].idScoping to $REVIEWER_LOGIN/$WORKFLOW_LOGIN, as the resolve step below does, would be tighter still — though it would strand a block left under the other identity, which the Bot test does not.
| # The shell and jq inside seidroid-review.yml, run against a gh stub, and its job | ||
| # conditions evaluated against synthetic payloads. Nothing here reaches the GitHub | ||
| # API, so this needs no token and no permissions. | ||
| # |
There was a problem hiding this comment.
[nit] "Five jobs" — there are six (place-findings, reactions, review-position, verdict-destination, review-limits, guard-admission). Worth correcting since the sentence's point is that the job count and the check list line up one-to-one.
|
Superseded by #108, which carries the workflow alone — one file, 3,934 lines. This pull request mixed the product with its test tooling: 45 test files and the CI job that runs them, almost exactly half the diff. Nothing is lost. The harnesses are on git checkout archive/seidroid-review-tests -- test/ .github/workflows/workflow-test-self.yml |


What this is
The first release of
seidroid-review.yml, the agentic pull request reviewer, as a reusable workflow. It reviews a pull request from inside the Actions runner and publishes what it found: inline comments on the changed lines, a check run, and a review position. A caller wires it withuses:and passes its own secrets.maincarries none of this work today, so the whole change is an addition: 47 files, 7,987 insertions, 0 deletions.Why one commit
This folds 33 commits from
feat/seidroid-reviewinto one. That branch was the integration target for PRs #71–#104; each was reviewed on its way in. The per-commit history is noise for a reader who has never seen the file, andmainhas no earlier version of it to compare against.The squashed tree is byte-identical to the branch tip.
What it does
The driver pin
The floor is
v0.21.0, in thedriver-versiondefault and inMIN_DRIVER_VERSION. Both name one release on purpose: the conclusion a review reaches for a given set of findings is specific to the driver that reached it, and a merge gate keyed on one is wrong for the others.v0.21.0was cut today (sei-internal-skills#453). It is the first release that renders the verdict from the structured block — summary first, inline findings counted in their section rather than repeated, nits folded — which is the body this workflow now publishes as the review.PLT-1268
PLT-1268 asked for inline comments, one artifact instead of two, and less verbosity. The driver half merged as sei-internal-skills#452. This is the uci half:
Place findings on the codesummary_posted/review_idsay whether a review carried itPlace findings on the codePost the verdictAdmit the requestThat last one is load-bearing. A run that places findings now posts no comment at all, so a guard reading only the comments would find nothing and review again on every push.
Tests
Seven harnesses, 739 assertions, 0 failures. Each reads the steps out of the YAML on every run, so none can pass against a stale copy.
workflow-test-self.ymlruns all seven.run.sh— placement and thread resolutionrun-guard.sh— the guard, and the reaction collectionconditions.py— the step conditionsreactions.sh— the three reaction stepsverdict.sh— where the verdict goes (new)decision.sh— which review event the position step recordsdeadline.py— the driver budget against the job capverdict.shis new here. It holds the step to the property the change is about: the review carries the verdict or a comment does, and never both. Its hardest cases are the ones where the append cannot happen — a review whose id could not be read, and an append the API refused. The verdict is on the pull request in both, so neither may fall back to a comment.shellcheck -S warningis clean on the new harness, and the three edited steps report no findings the tip did not already have.Not verified here
The workflow has not run end to end against
v0.21.0. The harnesses drive the extracted step scripts against aghstub; they do not call GitHub. First live proof will be this pull request's own review, once a caller points at the merge commit.After this merges
ucirelease.81e975conto that release tag.Until step 2 is done, please leave
chore/bump-seidroid-min-driver-v0-18-0and the tagpin-seidroid-min-driver-v0.18.0in place — the five callers pin that SHA directly.🤖 Generated with Claude Code