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

[bdchatham]

What this is

The first release of seidroid-review.yml, the agentic pull request reviewer, as a reusable workflow. It reviews a pull request inside the Actions runner and posts inline comments, a check run and a review position.

One file, 3,934 insertions, 0 deletions. It needs nothing else here — three pinned actions, the driver via go install, the diff through the API, and it never checks the caller out. Folds 33 commits from feat/seidroid-review; main carried none of it.

Driver pin, and PLT-1268

Floor v0.21.0, in both the driver-version default and MIN_DRIVER_VERSION: the conclusion a review reaches is specific to the driver that reached it. Cut today as sei-protocol/sei-internal-skills#453, with the driver half of PLT-1268 in sei-protocol/sei-internal-skills#452.

This is the uci half. The verdict now rides in the review holding the inline comments, a standalone comment is the fallback, and the guard counts a bot review bearing the marker as a prior review — without which a run that posts no comment re-reviews on every push.

Verification

739 assertions across seven (temporary scaffolding) harnesses, 0 failures, against this exact blob (09c86bd). They are not in this change — the tooling was half the diff — and are on archive/seidroid-review-tests. Nothing has run end to end against v0.21.0: the harnesses drive the step scripts against a gh stub, so first live proof is this pull request's own review.

After this merges

Cut a uci release, then move the five callers off 81e975c. Until then leave chore/bump-seidroid-min-driver-v0-18-0 and the tag pin-seidroid-min-driver-v0.18.0 in place — the callers pin that SHA.

🤖 Generated with Claude Code

[cursor]

PR Summary

Medium Risk
New org-wide review automation that mints credentials, posts merge-blocking reviews/check runs, and encodes subtle guard logic—bugs would affect PR gating for every caller that adopts it.

Overview
Introduces .github/workflows/seidroid-review.yml, a workflow_call reusable workflow that runs the sei-agent-driver in Actions to review pull requests without checking out the caller—diff and metadata come from the GitHub API, with go install pinned to v0.21.0 and a MIN_DRIVER_VERSION floor so older drivers cannot be used on review runs.

The wiring covers guard → in-cluster review → placement: admission (team/bot/fork/skip label, once-per-push gating with exceptions for re-review and block withdrawal), review / close session modes, inline findings, a check run, and a review event (approve / request changes / comment). The verdict lives in the bot review body (marker <!-- seidroid-review -->) with a standalone issue comment fallback; the guard treats that marker on bot reviews/comments as “already reviewed” so silent runs do not re-review every push.

Callers are expected to replace ai-review.yml, not run both; optional Seidroid GitHub App secrets gate comment-triggered reviews while automatic pull_request paths can run with GITHUB_TOKEN only where documented.

^{Reviewed by Cursor Bugbot for commit 9c7f209. Bugbot is set up for automated code reviews on this repo. Configure here.}

[seidroid]

A single new reusable workflow that is unusually careful about failure directions, token scoping, and injection — but the base-standards fetch uses gh api -f without --method GET, which silently turns the read into a POST so the Accepted list never reaches the driver. Two smaller correctness/consistency issues around block withdrawal and approving an unrecorded commit.

Findings: 1 blocking | 2 non-blocking | 0 posted inline

Blockers

• None at the file/PR level.
• 1 blocking issue(s) listed below under unanchored comments.

Non-blocking

• Nothing in the repository exercises this file (ai-review-self.yml still wires the old reviewer), so the first real validation happens in a downstream caller. Given the number of interacting failure paths, consider landing a caller in this repo — or at least a mode: close-only smoke job — in a follow-up so regressions in the guard/publish ladder surface here rather than in consuming repos.
• mode is a free-form string: a caller that passes anything other than review or close skips the guard and the review job with no annotation anywhere, reporting a green run that reviewed nothing. The Refuse an event this workflow does not handle step exists for exactly this class of caller error on the event axis; the mode axis has no equivalent.

Comments that couldn't be anchored to the diff

• .github/workflows/seidroid-review.yml:1893 -- [blocker] gh api defaults to GET only when no parameters are supplied; adding -f ref="$base" flips the method to POST, and the params then go in the JSON body rather than the query string. POST /repos/{owner}/{repo}/contents/{path} is not a defined route, so this call returns 404 on every run: $out is emptied, the *"HTTP 404"* arm is taken, and the log states $REPO has no $file on $base; nothing is accepted even when the file exists and is readable.

The net effect is that --base-standards-file / --base-standards-ref are never passed, so the repository's Accepted list never reaches the driver and every pre-existing blocker withholds approval — with no warning distinguishing this from a repository that genuinely has no standards file.

This workflow already documents the rule at line 1127 ("-F on a gh api that names no -X makes the request a POST, and this one only ever reads"), which is why the page size rides in the path there. Add --method GET here (-f is then correctly placed on the query string and encoded, which is what the comment above assumes).

• .github/workflows/seidroid-review.yml:2953 -- [suggestion] The dismissal selector matches on state == "CHANGES_REQUESTED" and the body marker alone — it never checks who recorded the review. Any review whose body starts with <!-- seidroid-review --> is dismissed, including one left by a human or by another bot that copied the marker (for example by quoting a previous verdict at the top of their own review body).

That is looser than every other ownership test in this file: the guard's prior-verdict read at line 1141 requires .user.type == "Bot", and Resolve the threads this review closed goes as far as requiring REVIEWER_LOGIN exactly, on the stated principle that "closing one is a mutation, and it stays as narrow as the identity performing it." Dismissing someone else's blocking review is a strictly larger mutation than resolving a thread.

Suggest adding the same login test the resolve step uses:

select(.state == "CHANGES_REQUESTED"
       and ((.body // "") | startswith($ENV.VERDICT_MARKER))
       and ((.user.login // "") | . == $ENV.REVIEWER_LOGIN or . == $ENV.WORKFLOW_LOGIN))

• .github/workflows/seidroid-review.yml:2839 -- [suggestion] When Record the commit under review fails (it is continue-on-error), head_sha is empty and commit_id is omitted, so the API defaults the review to the pull request's current head. For COMMENT and REQUEST_CHANGES that is the benign weaker guarantee the comment above describes. For APPROVE it is not symmetric: a push landing during the review receives an approval for code no reviewer read, and on a repository with approve-on-success: true plus an approval-count merge rule, that approval is load-bearing.

This also disagrees with how the same missing value is treated one step up: Publish the review check run deliberately declines continue-on-error and fails the job there, on the grounds that a merge gate which cannot name its commit should not exist at all. An approval is the same kind of gate.

Suggest gating just the approve arm on the recorded sha — e.g. downgrade APPROVE to no position (or to COMMENT) and warn when head_sha is empty, leaving the other two events on the documented fallback.

• .github/workflows/seidroid-review.yml:3722 -- [nit] <!-- seidroid-review:no-verdict --> is spelled here and again at line 3165 as NO_VERDICT_MARKER in Post the verdict. That is the exact drift hazard the header essay at lines 497–534 describes for VERDICT_MARKER and FINDING_MARKER — and it fails the same silent way: this step keeps stamping the old value while Post the verdict stops matching it, so a stale "this review did not complete" notice is never withdrawn and sits on the pull request beside a verdict that did land.

Hoisting it to the workflow-level env block beside the other two markers gives it one definition, as FINDING_MARKER already has for the same reason.

[cursor]

Stale Bugbot comment from a previous run.

[bdchatham]

Addressed in 6d0b1c3. Thanks — the blocker was real and the two suggestions were both right.

Blocker: the base standards read was a POST. Confirmed from gh's own help: "The default HTTP request method is GET normally and POST if any parameters were added." So -f ref= flipped it, the ref went into a JSON body, and POST /contents is not a route. Every run took the 404 and logged "no standards file" for a repository that had one. Fixed with --method GET.

The file already stated this rule for the guard's page size, and stated it one line above this very call — and the call still had the bug. So the fix is not only the flag: conditions.py now carries a file-wide rule that a gh api sending -f/-F must name -X or --method, stated over the file so it covers a step added later. gh api graphql is exempt. Reverting the flag fails that rule and nothing else.

Dismissal ownership. Agreed, and it was the loosest ownership test in the file. It now takes the same two logins the history read admits.

Approving an unrecorded commit. Agreed on the asymmetry. Without the recorded sha the approve arm downgrades to COMMENT and warns; REQUEST_CHANGES and COMMENT keep the documented fallback, since both stay true of code nobody read. Covered by three new decision.sh cases.

NO_VERDICT_MARKER drift. Hoisted to the workflow env beside the other two.

mode is free-form. Fixed. The guard condition admitted review only, so a typo skipped both jobs and reported green — the refusal could not run because the job carrying it never started. The condition now admits any non-close mode, and Refuse an event this workflow does not handle names the mode and refuses it.

Nothing in the repository exercises this file. Correct, and deliberate for this PR: it carries the product alone, and the harnesses were almost exactly half the original diff. They are on archive/seidroid-review-tests — seven harnesses, 760 assertions, all passing against this head. Landing a caller or a mode: close smoke job here is the right follow-up and I have noted it.

Bugbot's high-severity finding (a submitted review's body cannot be updated) is not reproducible; evidence in the thread on that comment.

[cursor]

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

There are 2 total unresolved issues (including 1 from previous review).

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 6d0b1c3. Configure here.}

[bdchatham]

@seidroid review

[bdchatham]

@seidroid review

Two rounds of fixes since your last pass, at d4b48ae:

• The blocker: Read the base branch's accepted conditions now uses --method GET, so the Accepted list actually reaches the driver.
• The dismissal tests who recorded the review it withdraws.
• The standing-block gate now names only what that dismissal will dismiss — Bugbot caught the disagreement my first fix introduced.
• An approval requires the recorded commit, or downgrades to COMMENT.
• NO_VERDICT_MARKER is hoisted to the workflow env.
• An unrecognised mode is named and refused, and the guard condition admits it so the refusal can run.

Your non-blocking point about nothing in this repository exercising the file still stands and is deliberate for this PR — it carries the product alone. The harnesses are on archive/seidroid-review-tests: 764 assertions across seven, passing against this head.

[seidroid]

Claude finished @bdchatham's task in 2m 54s —— View job

---

COMMENTED

The blocker is gone and every item from the last pass is answered at d4b48ae. One new finding, non-blocking: the gate/withdrawal disagreement Bugbot named is narrowed, not closed, and the case that remains now fails silently where it used to fail loudly.

Findings: 0 blocking | 1 non-blocking | 0 posted inline

Verified against the file at d4b48ae

Item	Where	Status
Base standards read is a POST	--method GET at :1942	fixed
Dismissal ownership	login test at :3032-3036	fixed
Standing-block gate disagrees with it	.user.type == "Bot" at :1138	fixed, see below
APPROVE on an unrecorded commit	downgrade to COMMENT + warning at :2830-2840	fixed
NO_VERDICT_MARKER drift	one definition at :540, three readers, no step-level shadow	fixed
mode free-form	refusal at :693-707, guard condition widened to inputs.mode != 'close'	fixed

The mode fix holds on each axis I could walk: a typo on pull_request now starts the guard, fails the refusal and skips the review job (whose own clause still names == 'review'); a typo on a comment event fails the same way; pull_request + close still skips the guard, and the review job still reads that skip as its trigger. The approve downgrade is genuinely load-bearing rather than belt-and-braces — Publish the review check run has already failed the job by then, but the position step runs on !cancelled() and still reaches the API.

Non-blocking

The withdrawal can decline a block the gate committed a run to clearing, and say nothing. The gate at :1138 admits any Bot whose CHANGES_REQUESTED body opens with VERDICT_MARKER; the withdrawal at :3032 additionally requires REVIEWER_LOGIN or WORKFLOW_LOGIN. The comment argues the asymmetry is pointed the safe way, and for the quoting case it is. Two routes still separate them:

• A rotated App credential. The SEIDROID_APP_ID description documents this path directly — "dismiss any standing block by hand when you change the credential." A block recorded under the old slug matches the gate and fails the withdrawal's login test.
• Name the identity this review posts under failing. It is continue-on-error: true, so nothing reaches $GITHUB_ENV, $ENV.REVIEWER_LOGIN is null to jq, and (.user.login // "") | . == null is false for every review.

Either way ids comes back empty, the loop skips on [ -n "$id" ] || continue, and the step reports superseded blocks: 0 withdrawn, 0 still standing and exits 0 on a green job — while the gate, which did find the block, re-reviews on every push for the life of the pull request, one sandbox and one quota spend each. Before 6d0b1c3 the dismissal was at least attempted: on an unprotected branch it succeeded (same tool, different credential — the right outcome), and on a protected branch with dismissal restrictions the refusal produced the ::error:: that tells the operator what the secret description asks them to do.

Worth noting too that the threads step already warns for exactly the second route — "open with this tool's marker and none was written by … so this review carries no history and can close no thread" — and the withdrawal, the one step in the job that clears a merge gate, has no equivalent. The identity step's own header is now a little short of the truth as well: it says a failure there "costs the history and the thread closing", and it now also costs the withdrawal.

The narrow fix keeps the mutation exactly as narrow as it is and restores the signal — count what the marker matched before the login test, and report the difference rather than swallow it:

# shellcheck disable=SC2016  # $ENV is jq's own, and jq reads it
marked="$(gh api "repos/$REPO/pulls/$PR/reviews" --paginate \
            --jq '[.[] | select(.state == "CHANGES_REQUESTED"
                         and ((.body // "") | startswith($ENV.VERDICT_MARKER)))] | length')" || marked=""

then after the loop, where marked exceeds withdrawn + stuck, name the difference in an ::error:: and count it toward stuck — which already exits 1. What the gate calls a block from this workflow and the withdrawal will not touch is the one state neither reader can currently report.

Fix this →

What this pass could not check

The file parses and the shell in the regions that changed reads correctly, but nothing here exercises it — the harnesses are on archive/seidroid-review-tests and I did not run them, so the 764 assertions are your report rather than mine. Your standing note on that is fair for this PR. One thing I did confirm: the mangled review"' "$CHECK" fragment visible in the quoted diff context of the earlier threads is an artifact of that rendering, not the file — :2652 reads title="$(jq -r '.title // "review"' "$CHECK" || true)" and the YAML loads clean.
· branch release/seidroid-review-workflow

[bdchatham]

@seidroid review

[seidroid]

A single new reusable workflow that lands every blocking item from the previous review — the --method GET fetch, the dismissal login test, the approve-arm commit gate, the hoisted NO_VERDICT_MARKER and the mode-axis refusal are all in the current head. What remains is three narrower gaps in the publish/recall ladder plus the release-ordering dependency on a driver tag that has not been cut in this repository.

Findings: 0 blocking | 3 non-blocking | 0 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• Nothing in this repository exercises this file — ai-review-self.yml still pins ai-review.yml@1d50ef2, and no caller references seidroid-review.yml. The first real validation of the guard/publish ladder happens in a downstream caller. Landing a caller here, or at least a mode: close-only smoke job, would surface a regression in this repository rather than in a consuming one. (Restating the same point from the previous review; still true at this head.)
• driver-version and MIN_DRIVER_VERSION both pin v0.21.0, and the description says that release was cut today as sei-protocol/sei-internal-skills#453. Confirm the sei-agent-driver/v0.21.0 tag is published and served by proxy.golang.org from a COLD module cache before merging — the install step has no fallback, so an unpublished tag fails every review at go install, on every caller, at once. The driver-version docstring already asks for exactly this cold-cache verification; it applies to the default too.
• The 739 assertions live on archive/seidroid-review-tests and not in this change, so nothing in this repository re-runs them when this file is edited. Given how much behaviour rides in the step scripts, consider a follow-up that brings the harnesses in-tree behind a workflow that runs on changes to this path.

Comments that couldn't be anchored to the diff

• .github/workflows/seidroid-review.yml:1647 -- [suggestion] threads_path is published here, before $THREADS is written at line 1735 — and $THREADS is a fixed name in RUNNER_TEMP (review-prior-threads.json) with no run id in it.

If the GraphQL fetch at 1674 fails, set -euo pipefail aborts the step (tolerated by continue-on-error) having truncated only $THREADS.raw. $THREADS itself is never touched, so it keeps whatever a previous run of this step left there. The output is already set, so Drive session + collect verdict sees [ -s "$THREADS" ] succeed and passes --conversation-context pointing at another pull request's prior findings, presented to the review as its own history. Silent in both directions: the fetch failure is a tolerated step, and the driver reads a populated file as a successful recall.

This is the same hazard this file defends against three times elsewhere, on the same stated premise — rm -f "$out" before the base-standards write (1911–1912, "RUNNER_TEMP survives between jobs on a non-ephemeral self-hosted runner, which is the steady state here"), rm -rf "$out_dir" in the drive step (2015–2017), and : > "$LINKAGE" in placement (2153–2157). This is the one read path that does not.

One line fixes it: rm -f "$THREADS" "$THREADS.raw" immediately after the threads_path echo, or write into the run-scoped directory the drive step already builds. The second jq at 1735 needs nothing — its own > truncates before it can fail.

• .github/workflows/seidroid-review.yml:2032 -- [suggestion] The driver is invoked with the repository and the pull request number only — it resolves the head itself (its prompt's first step is gh pr diff) — so nothing binds it to the sha Record the commit under review captured at 1806. The install contract check at 1530–1532 confirms no sha flag exists on the surface this file drives.

That makes the comment at 1803–1808 ("This is the commit the driver is about to review, and the commit every publisher names") stronger than what holds. A push landing between the record and the driver's fetch means every publisher names commit A while the review read commit B: the placement batch is partitioned against A's compare output while the model's line numbers come from B, so anchorable findings either land on the wrong lines or lose the whole batch to a 422 and fall to the summary; and under approve-on-success the approval is recorded against a commit the review did not read — the case the approve arm at 2832 was just hardened for on the missing-sha axis but not on the stale-sha axis.

The window is small (seconds, not the whole review) and re-review-on-push off means a push does not cancel the run, so it will not self-correct. A cheap mitigation entirely inside this file: re-read the head after the drive step and compare it with steps.head.outputs.sha; if it moved, warn, and downgrade the APPROVE arm the same way the empty-sha branch already does. That leaves the check run and the comment on the documented weaker guarantee and protects only the assertion that cannot be walked back.

• .github/workflows/seidroid-review.yml:2631 -- [suggestion] An absent check.json exits 0 and publishes nothing. The step comment at 2602–2604 states this deliberately ("a run that never reached the driver still publishes nothing"), but it has a consequence on the re-review path that the reasoning does not cover.

Check runs are keyed on the commit, and GitHub surfaces the latest one per name. So on head commit X: an automatic review publishes review = success, then a @seidroid review re-review on the same X dies before the driver — a failed go install, an unresolvable toolchain, the MIN_DRIVER_VERSION refusal at 1509 — and this step writes nothing. The success from the first run stays the current review (and AI Review) check on X, so a branch-protection rule requiring either reads green for a review that did not happen. The Report a review that reached no verdict comment is the only trace on the pull request, and the check that gates the merge disagrees with it.

Note that Post the verdict's own failure path (3495–3511) already does the right thing here — it publishes a failure check under both names precisely so a stale conclusion cannot stand. The same treatment fits this branch: when $CHECK is absent and REVIEWED_SHA is non-empty, publish failure under review (and the AI Review copy) with a title naming the run, so the gate reflects the run that just failed rather than the one before it.

Superseded: latest AI review found no blocking issues.

[bdchatham]

All three suggestions from the last review are fixed in ce8bf94, and the release-ordering question is answered.

The recall file could be another pull request's. Correct, and the worst of the three. threads_path was published before $THREADS was written, the name is fixed in RUNNER_TEMP with no run id, and a failed fetch aborted the step with the output already set. The drive step's -s test then passed on whatever the last run of this job left there. Now emptied where its path is published.

That one earned a rule rather than a line. conditions.py gains a second file-wide check: a run block publishing <name>_path=$VAR must clear $VAR in the same block — by name, or by a directory it is assigned under and that the block rm -rfs. The drive step passes on the second form and is the stronger of the two, since its directory is run-scoped as well. Reverting the THREADS clear fails that rule and nothing else.

An absent check file left the previous run's conclusion standing. Correct. It now publishes failure under both names, as Post the verdict's failure path already did for the same hazard. With no recorded commit there is no gate to correct, so that case still publishes nothing.

An approval outliving the commit it read. Correct, and a real gap in the hardening I did last round — I closed the missing-sha axis and left the stale-sha one open. The position step now re-reads the head and downgrades APPROVE to COMMENT when it moved. Only the approval is gated on it, for the reason the missing-sha branch gives. A read that fails leaves the vote standing: the window is seconds, and withholding on an unanswered question would cost the common case to guard the rare one. Four new decision.sh cases.

The driver tag. Verified from an empty GOMODCACHE: sei-agent-driver/v0.21.0 resolves through the proxy and builds in 6.1s.

Still open, by choice. Nothing in this repository exercises the file, and the harnesses are not in it. Both are the consequence of this PR carrying the product alone — they are on archive/seidroid-review-tests, now 776 assertions. A caller here, or a mode: close smoke job, is the right follow-up.

One gap stated plainly: the check-publish change has no automated coverage. No harness drives that step, and I did not build one for this round.

[masih]

Review feedback generated by Devin Review

Here's my assessment of the three flagged bugs against the actual code.

Bug 1 — Concurrency key includes mode (.github/workflows/seidroid-review.yml:1304-1306)

The flag says including mode lets a review and a close run at once for the same PR, and close could disrupt/outrun the review. But the code comment at :1294-1297 states the opposite intent deliberately: if mode were not in the group, commenting on a just-closed PR would cancel the in-flight close (cancel-in-progress: true), and close is "the only thing that reclaims a sandbox."

So this is a genuine trade-off, not a clear bug. The author chose to protect close from cancellation at the cost of allowing review+close concurrency. Whether that's wrong depends on driver session semantics (does close deleting a session mid-review corrupt anything?). I can't verify the driver side from this repo. Verdict: design trade-off, needs driver-side confirmation — not a clear-cut bug.

Bug 2 — Stale block withdrawal after head moves (.github/workflows/seidroid-review.yml:3034-3037)

This one looks real. The moved check (:3034 region, and the moved=true logic around :3030 in the earlier read) downgrades an approval to a comment when the head moved during review. But the withdrawal logic only exits early on event = REQUEST_CHANGES, conclusion = failure, or conclusion = unknown (:3034-3035). It does not consult moved.

So: review reads commit A, concludes clean, head moves to B mid-review → approval is correctly downgraded, but the withdrawal at :3034+ still proceeds and dismisses a standing CHANGES_REQUESTED block. That clears a block against commit B, which was never reviewed. The asymmetry the author documents for approval (an approval "asserts something about a specific diff") applies equally to withdrawing a block — a dismissal also asserts the current head is clean. Verdict: plausible real bug. Worth flagging.

Bug 3 — No-verdict notice cleanup only deletes the last one (.github/workflows/seidroid-review.yml:3526-3528)

Also real, but low severity. The cleanup pipes through tail -n 1, deleting only the most recent NO_VERDICT_MARKER comment. The step's own comment at :3512-3515 claims "exactly one" notice invariant. That invariant holds only if every prior run successfully deleted its predecessor. If a deletion ever failed (the tolerated-failure path at :3532-3534), duplicates accumulate and this cleanup removes just one per successful run. The parallel step "Report a review that reached no verdict" has the same tail -n 1 pattern (in delete_prior_notice).

The fix is to delete all matching notices, not just the last. Verdict: real but non-severe, matching the flag.

---

Summary you can paste into the PR

Bug 2 (severe): stale block cleared when head moves during review. In State the review's position (.github/workflows/seidroid-review.yml), the moved check downgrades an approval to a comment when the PR head changes mid-review, but the block-withdrawal logic at the end of the step (:3034-3037) gates only on event/conclusion, never on moved. A clean review of commit A can therefore dismiss a standing CHANGES_REQUESTED block after the head has moved to an unreviewed commit B. A dismissal asserts the current head is clean, so it should be subject to the same stale-head guard as the approval.

Bug 3 (non-severe): duplicate no-verdict notices survive. The stale-notice cleanup in Post the verdict (.github/workflows/seidroid-review.yml:3526-3528) and in Report a review that reached no verdict uses tail -n 1, deleting only the newest NO_VERDICT_MARKER comment. If any earlier deletion failed (a tolerated path), duplicates accumulate and only one is removed per run, leaving stale "this review did not complete" notices on a PR whose review did complete. Delete all matching notices, not just the last.

Bug 1 (needs driver confirmation): review/close concurrency. Including mode in the concurrency group (:1304-1306) is a deliberate trade-off — it protects an in-flight close from being cancelled by a later comment — but it does allow a review and a close for the same PR to run simultaneously. Confirm on the driver side that a close deleting a session while a review turn is mid-flight is safe.

[bdchatham]

Thanks — your read on all three was right, including the one you declined to call a bug.

Bug 2 — fixed in 5b33c1e. You are correct, and it was mine: ce8bf94 added the moved check and wired it to the approval alone. A dismissal asserts the current head is clean, which is the same class of claim, so it now stops on a moved head. The block standing is the recoverable direction — the next review clears it, where a block wrongly cleared needs a human to put back.

One detail worth recording: my own test had encoded the defect. The approve, head moved case asserted the withdrawal proceeded. The pair now isolates the guard — the same clean review withdraws on an unmoved head and does not on a moved one.

Bug 3 — fixed in 5b33c1e, both sites. Post the verdict and Report a review that reached no verdict each take every match now instead of tail -n 1. Your diagnosis of why the "exactly one" invariant was false is exactly it: it holds only while no deletion has ever failed, and that path is tolerated in both. Five new verdict.sh cases, with the stub serving a list of notices and logging each delete by id.

Bug 1 — filed, not changed here. PLT-1274.

You framed it correctly as a trade-off rather than a bug, and I agree it should not be settled from this repository. Worth stating the shape so the ticket is not re-derived: keeping mode out of the group is wrong in a known way — cancel-in-progress: true means a comment on a just-closed pull request would cancel the in-flight close, and close is the only thing that reclaims a sandbox. So it is a certain leak versus an unknown interaction, and the file took the second. If the driver turns out to be unsafe under a mid-flight session delete, the fix belongs on the side that owns the session, because a uci-side fix costs the cancellation guarantee close currently has.

Each fix was checked against its defect: reverting either fails only the assertions written for it. 781 assertions across the seven harnesses, on archive/seidroid-review-tests.

Separately, the header comment on the workflow is down from 71 lines to 12 (86fcf9d). What it said about the two paths, the four events, the allow-tools default and the injection routing is stated at each of those places already, and the "what changed versus the Python driver" section was history, which belongs in a pull request and not in the file.

[bdchatham]

Peer-reviewed 5b33c1e with three independent passes before asking anyone to look again. They found the fix incomplete, and one claim in my own commit message wrong. Both are addressed in 9c7f209.

The fix for Bug 2 closed one branch of three. moved was a boolean, so a head that could not be CHECKED read as a head that had not moved. Two other branches reached the dismissal that way: a commit the recording step never wrote, and a head re-read that failed or returned a non-sha. Either cleared a standing block with nothing having confirmed the diff it now guards — the unverified-clean claim my own commit message argued needs a human.

It is three states now. same, moved, unknown — because the two publishers want opposite things from the third. The approval treats unknown as good enough, deliberately: a wrong approval is corrected by the next review. The withdrawal cannot, because nothing here puts a cleared block back. Carrying the approval's rule across was the same mistake twice — the two differ in whether their error heals, which is the whole argument for the guard.

Correction to 5b33c1e's message. It said "reverting either fails only the assertions written for it". That held for the dismissal and for the verdict step's cleanup. It did not hold for the cleanup in Report a review that reached no verdict: nothing exercised that step at all, so reverting it failed zero assertions. The sentence implied coverage that did not exist, and I verified only one of the two sites before writing it. I cannot amend a pushed commit, so the correction is in 9c7f209's message.

That site is now driven directly — reverting it fails four assertions. The stub also gained a delete-failure knob, so the tolerated path that makes duplicates possible is reachable by a case rather than only argued about in a comment.

Also fixed: the comment above the head computation still said only the approval was gated on it; three comments still described one no-verdict notice where the code takes every one; and the concurrency trade-off now cites PLT-1274 rather than claiming the question is tracked somewhere unnamed.

Confirmed not worth changing: head_sha is captured at job start while the driver resolves the head itself, so moved can fire when the driver did read the current head. Costs one review cycle and self-heals. It does name the real root — nothing pins the driver to a sha — which is adjacent to PLT-1274 and not a one-line fix here.

PLT-1274 also now records an interim mitigation, if the driver answer is slow: the guard can refuse admission while a run of the opposite mode is in flight. It is check-then-act, so it narrows the window rather than closing it.

796 assertions across seven harnesses. One note on the suite itself: a chained run reported spurious run-guard.sh failures once during the review. I could not reproduce it in three chained attempts, so it is unexplained rather than resolved.

[bdchatham]

@seidroid — is Masih's feedback on this pull request addressed? Please verify against the code at HEAD (9c7f209) rather than taking my summary for it, and say which items you consider closed and which not.

His review left three findings:

1. Bug 1 (needs driver confirmation): mode in the concurrency group lets a review and a close run at once for the same pull request. He called it a deliberate trade-off, not a bug, and asked for driver-side confirmation that a close deleting a session mid-review is safe.
2. Bug 2 (severe): the moved check downgraded an approval when the head moved mid-review, but the block withdrawal gated only on event/conclusion and never consulted it — so a clean review of commit A could dismiss a standing CHANGES_REQUESTED after the head moved to an unreviewed commit B.
3. Bug 3 (non-severe): both no-verdict notice cleanups used tail -n 1, deleting only the newest notice, so duplicates survived if any earlier deletion had failed.

What I claim, and what I would like checked:

• Bug 2 is fixed across two commits. 5b33c1e gated the withdrawal on moved. That was incomplete: moved was a boolean, so an unrecorded commit or a failed head re-read read as "not moved" and still cleared the block. 9c7f209 replaced it with three states — same, moved, unknown — where the approval treats unknown as good enough and the withdrawal requires same. Please check the withdrawal cannot be reached on any unconfirmed head, and that the asymmetry between the two publishers is justified rather than just asserted.
• Bug 3 is fixed at both sites named — Post the verdict and Report a review that reached no verdict. Please check the selection was not widened in the process, so a person's comment carrying the marker still cannot be deleted.
• Bug 1 is deliberately not changed. It is filed as PLT-1274 and the concurrency block now cites it. Please say whether you think punting it is defensible from what this file can show, or whether there is a cheap mitigation here I have missed.

Also worth your attention, since it is the newest code and no reviewer has seen it: the three-state head check in State the review's position on the pull request, and whether anything else in that step or elsewhere publishes an assertion about a commit it did not confirm.

[seidroid]

Claude finished @bdchatham's task in 3m 38s —— View job

---

COMMENTED

Verified against the file at 9c7f209, not against your summary. Bug 2 and Bug 3 are closed. Bug 1 is defensibly punted — I could not find a cheap in-file mitigation that keeps both properties, and I'll say why below. Three non-blocking findings, two of them in the newest code.

Findings: 0 blocking | 3 non-blocking | 0 posted inline

Bug 2 — closed

What I checked	Where	Result
head_state starts at the unsafe value	:2817 — head_state=unknown before any branch	holds
It reaches same only on a confirmed equal sha	:2818-2828 — needs non-empty head_sha, a read that returned, a value passing *[!0-9a-f]*, and "$now" = "$head_sha"	holds
The withdrawal requires same	:3039 — if [ "$head_state" != same ]; then … exit 0	holds
The gate precedes the listing and the loop	gate :3039-3045, listing :3076	holds
Nothing can skip the assignment	:2817 is unconditional in the same run block as the withdrawal (step spans :2696-3104)	holds
The re-read cannot abort the step	now="$(gh api … 2>/dev/null || true)" — the step has no continue-on-error, so an abort here would have silently skipped the withdrawal	holds

So: the withdrawal is unreachable on an unrecorded commit, on a failed re-read, on a non-sha answer, and on a moved head. That is every branch Masih's finding and your own follow-up named.

On the asymmetry — justified, but for a different reason than the comment gives. There is a real, checkable distinction between the three publishers, and it is not the one at :2808-2816:

• APPROVE must be guarded because an approving review counts toward a required-approvals rule for the pull request, not per commit. The push has already landed by the time this step runs, so "dismiss stale approvals on push" cannot retract it — an approval whose commit_id is A is load-bearing for B.
• The dismissal must be guarded for the same reason: clearing a CHANGES_REQUESTED clears the gate for whatever is at head, which is B.
• The check run needs no guard, and correctly does not have one at :2645-2647: a required check is evaluated on the pull request's current head, so a success published against A does not satisfy a rule on B — it reads pending. Safe direction, which is why the guard stops where it does.

Where the comment overreaches is :2809 and :3035: "a wrong approval is corrected by the next review." With re-review-on-push off and no block standing, no next automatic review runs — the guard's re-review exemption at :1075-1090 fires on a standing block or on the input, and a wrong approval leaves neither. So nothing corrects it; the approval sits there. The conclusion still stands on the other half of the argument you make at :3036-3038 — leaving a block costs a review cycle a later run can spend, while clearing one costs a gate nothing in this file can restore — and that half needs no assumption about a next review. Worth trimming the sentence rather than the code.

Bug 3 — closed, and the selection was not widened

Diffed 5b33c1e^..HEAD across both sites. The only change to either selector is the removal of | tail -n 1; the two ownership tests are byte-identical to what they were:

select(.user.type == "Bot") | select(.body | startswith($ENV.NO_VERDICT_MARKER))

• Post the verdict — :3504-3514
• Report a review that reached no verdict — delete_prior_notice, :3865-3882

A person's comment quoting the marker still fails .user.type == "Bot", and one that mentions it without opening on it still fails startswith. Both loops now take every match and warn per failure rather than per run. The exit status moved from tail's to gh's, and the || true at :3506 and :3873 covers that — checked, because without it a listing failure would abort Report a review that reached no verdict under set -e before the notice posts.

Bug 1 — punting is defensible; I could not find the cheap fix

The trade-off is stated in the file now, in both directions, with the ticket (:1235-1243). I walked the alternatives and none of them is cheap:

• Drop mode from the group, keep cancel-in-progress: true. GitHub takes cancel-in-progress from the incoming run, so an arriving review cancels an in-flight close. That is the certain sandbox leak you already rejected, pointed the other way.
• Drop mode, set cancel-in-progress: false. Serialises review against close — but also makes a new review queue behind an old one instead of superseding it, which costs "latest wins, never two posters" and means two reviews post on one pull request.
• cancel-in-progress: ${{ inputs.mode != 'close' }} on a shared group. Expressions are allowed there, but it buys nothing: the close queues politely and the review still cancels the close.

So there is no configuration of one concurrency block that serialises the two without giving up either cancellation or single-poster. The only in-file options are procedural, and the one you recorded on the ticket (refuse admission while the opposite mode is in flight) is check-then-act as you say.

One variant you may not have considered, since it narrows differently: rather than the guard refusing a close, have the close path wait — poll gh api "repos/$REPO/actions/runs?status=in_progress" for a review run on this pull request and sleep until it clears, before invoking the driver with --close. Still check-then-act, still racy in the last instant, but it costs no review and no refusal, and close is the one path with no latency requirement. It is strictly a narrowing of the window, not a fix, so it belongs on PLT-1274 beside the other mitigation rather than in this pull request.

Non-blocking, in the newest code

1. The thumbs-up is the one reader-facing signal that still asserts a commit this run knows it may not have read. Answer the request picks its reaction from conclusion alone (:3185-3191): success earns +1. head_state never reaches it — State the review's position on the pull request carries no id (:2696), so it publishes no output, and the reaction step has nothing to consult. So on a moved head the position step declines to approve and the withdrawal declines to clear, while the person who asked gets a 👍 on their comment. The step's own comment at :3173-3175 sets the rule this breaks: "a thumb up there says the change is fine when nobody looked at it." A moved head is a weaker version of exactly that. The *) arm already exists for "earns no reaction", so the fix is an id: on the position step, a head_state output, and one extra term on the success arm.

Fix this →

2. On a moved or unconfirmed head, nothing on the pull request says so on most paths. The only on-PR statement is the approve arm's note at :2866, which needs decision=approve and approve-on-success: true. With approval off, event stays empty, no position is recorded, and the moved warning lives only in the run log — so the author reads a clean verdict beside a block that did not clear, with no artifact explaining the gap. This matches how the counts-based refusals at :3020-3027 already behave, so it is a consistency observation rather than a regression; but the withdrawal is the one place where the silence costs a merge gate, and a line appended to the verdict body would close it.

3. .body is read without the // "" the rest of the file uses. Both cleanups say select(.body | startswith($ENV.NO_VERDICT_MARKER)) (:3505, :3872), where the guard's reads and the withdrawal selector at :3078 all say (.body // ""). jq raises on a null body, the whole listing is swallowed by the || true, and the run withdraws zero notices while reporting nothing — the silent direction the rest of the file is written against. Not reachable today: the issue-comments endpoint always sends a string body. It is one token, and it makes the two sites read like their neighbours.

What this pass could not check

The driver half of Bug 1 — whether a session delete mid-turn is safe — is not answerable from this repository, which is the right reason for PLT-1274 to exist. And nothing here exercises the file: the harnesses are on archive/seidroid-review-tests and I did not run them, so the 796 assertions are your report rather than mine. I did confirm the specific claim you flagged as previously wrong: Report a review that reached no verdict is now reachable as a driven site, since delete_prior_notice is called unconditionally at :3920 ahead of the post, so a revert of the tail -n 1 removal there is observable rather than only arguable.

· branch release/seidroid-review-workflow ·

[masih]

LGTM