Skip to content

feat(seidroid-review): add the agentic pull request reviewer as a reusable workflow - #107

Closed
bdchatham wants to merge 1 commit into
mainfrom
release/seidroid-review
Closed

bdchatham wants to merge 1 commit into
mainfrom
release/seidroid-review

Conversation

@bdchatham

Copy link
Copy Markdown
Contributor

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 with uses: and passes its own secrets.

main carries 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-review into 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, and main has no earlier version of it to compare against.

The squashed tree is byte-identical to the branch tip.

What it 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 — this is the PLT-1268 change. 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 pin

The 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 a merge gate keyed on one is wrong for the others.

v0.21.0 was 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:

Change Where
The review body is the driver-rendered verdict Place findings on the code
summary_posted / review_id say whether a review carried it Place findings on the code
Append to that review, or post a comment when none carried it Post the verdict
A bot review with the marker counts as a prior automatic review Admit the request

That 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.yml runs all seven.

Harness Assertions
run.sh — placement and thread resolution 290
run-guard.sh — the guard, and the reaction collection 248
conditions.py — the step conditions 80
reactions.sh — the three reaction steps 77
verdict.sh — where the verdict goes (new) 26
decision.sh — which review event the position step records 9
deadline.py — the driver budget against the job cap 9

verdict.sh is 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 warning is 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 a gh stub; 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

  1. Cut a uci release.
  2. Move the five caller repositories off 81e975c onto that release tag.

Until step 2 is done, please leave chore/bump-seidroid-min-driver-v0-18-0 and the tag pin-seidroid-min-driver-v0.18.0 in place — the five callers pin that SHA directly.

🤖 Generated with Claude Code

…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>
@cursor

cursor Bot commented Sep 15, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
New automation that holds Omnigent/GitHub credentials and can block merges via review state; risk is mitigated by guard gates and extensive stub-based tests but live behavior is not yet verified against v0.21.0.

Overview
Introduces seidroid-review.yml as a workflow_call reusable workflow: callers trigger automatic pull_request reviews or manual @seidroid review comments, run the pinned sei-agent-driver (v0.21.0 default / floor), and publish results on GitHub (check run, review position, inline findings).

PLT-1268 behavior: the driver-rendered verdict is the body of the single COMMENTED review that holds inline findings when placement succeeds; Post the verdict only appends or posts a standalone comment when no review carried it (summary_posted / review_id). The guard treats prior bot reviews whose body starts with the verdict marker as an automatic review, so pushes are not re-reviewed endlessly after findings stop posting separate verdict comments.

Also adds admission rules (fork/draft/label/re-review), supersede-and-resolve thread handling, withdrawal of stale CHANGES_REQUESTED blocks, trigger reactions, and test/seidroid-review harnesses (including new verdict.sh) plus workflow-test-self.yml to exercise extracted step scripts against a gh stub.

Reviewed by Cursor Bugbot for commit f841f0c. Bugbot is set up for automated code reviews on this repo. Configure here.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ 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 ---"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit f841f0c. Configure here.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 conditions is the one step in the review job with no coverage in test/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 that path/ref are emitted for a file that exists would have caught the POST bug above.
  • Post the verdict and Report a review that reached no verdict both 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/-X makes this a POST, not a GET. gh api defaults to GET only while no parameters are supplied; the first -f flips it to POST and sends the params as a JSON body. This file already states that rule itself at line 1126 ("-F on a gh api that names no -X makes 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")"; then

The -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 on state == "CHANGES_REQUESTED" and the marker alone, with no test on who wrote the review. A human review that opens with this file's VERDICT_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)))] | .[].id

Scoping 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.
#

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@bdchatham

Copy link
Copy Markdown
Contributor Author

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. seidroid-review.yml needs none of it at runtime, so the tooling does not belong in the change that makes the reviewer usable across our repositories.

Nothing is lost. The harnesses are on archive/seidroid-review-tests (the tree this branch held), and the PLT-1268 work with its own history is on archive/seidroid-review-plt-1268. To bring the tests in later:

git checkout archive/seidroid-review-tests -- test/ .github/workflows/workflow-test-self.yml

@bdchatham bdchatham closed this Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant