fix(seidroid-review): publish a check run and a comment when a review reaches no verdict

[bdchatham]

Closes PLT-1143 (workflow half). The driver half shipped in sei-protocol/sei-internal-skills#405 and releases as v0.14.0.

Problem

A review that reaches no verdict publishes nothing. The pull request carries no comment, no check run, and no reaction that says anything happened. A reviewer sees a red job and an untouched pull request, and cannot tell a review that ran and could not be read from one that never ran. Only one of the two is a reason to look.

What changes

The check run publishes whichever check the driver wrote. The step dropped its verdict_produced gate; the file's presence is the gate instead. A no-verdict run writes a check under the title no verdict concluding failure, carrying the reason and no counts key. A run that never reached the driver still publishes nothing.

Read the title, not the conclusion, to tell the two apart — a decided review carrying blockers concludes failure too.

A new step reports the run in a comment. It is the exact complement of Answer the request, on one output: == 'true' there, != 'true' here. So a review run that was not cancelled posts one comment or the other, never both and never neither — including a run that died before the driver, where verdict_produced is unset and this is the only record left.

The comment quotes the check run's own summary, so the two cannot disagree. When the run stopped before the review started there is no reason to quote, and the body says that rather than implying the review ran.

The position step keeps its gate. verdict_produced is load-bearing there twice over: the withdrawal at the end reads the driver's finding counts, the no-verdict check carries none, and the no-counts branch proceeds by design. An ungated no-verdict run would clear a standing block on the strength of a review that produced nothing. The check run and the comment publish without the gate; anything that clears a merge gate keeps it.

Parity

ai-review.yml answers the same case the same way, and forces failure on unreadable output for the same reason.

Requires

Driver v0.14.0. On an older pin the check file is absent on a no-verdict run, the publish step self-gates on [ ! -s "$CHECK" ], and the comment step still reports — so this degrades rather than breaks.

Verification

python3 -c "import yaml; yaml.safe_load(...)"   parses
actionlint base   4 [shellcheck]
actionlint head   4 [shellcheck]   unchanged
grep review_repo\|review_pr        0    (the cross-repo grammar #83 removed stays removed)

Not verified from here: whether the new step's condition fires as written in GitHub's runner. That needs a live no-verdict run, which the next @seidroid review after the pin bump will produce.

[cursor]

PR Summary

Medium Risk
Changes merge-gate check conclusions, PR comments, and trigger reactions for failed reviews; position/withdrawal stays gated to avoid clearing blocks on empty runs.

Overview
When a seidroid review finishes without a verdict, the workflow now leaves a visible record on the PR instead of a red job with nothing on the pull request.

The drive step treats “no verdict” as a failed job (exit 5 if the driver exited 0). Publish the review check run no longer requires verdict_produced; it posts whenever check.json exists and forces conclusion=failure when there was no verdict so a partial/crashy driver file cannot green a required check. Answer the request only maps check conclusions to thumbs when a verdict existed, so a no-verdict failure is not read as “the change is bad.”

A new step Report a review that reached no verdict posts a single bot comment (marker <!-- seidroid-review:no-verdict -->) with the check summary and run link; Post the verdict deletes that notice when a later run succeeds. State the review's position still gates on verdict_produced so a failed read cannot dismiss an earlier block.

Requires driver v0.14.0 for the no verdict check artifact; older pins degrade to comment-only reporting.

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

[github-actions]

The change is well-scoped and the reasoning about which publishers keep the verdict_produced gate is sound; no blocking defects. Two notes: dropping the gate on the check-run publish step means it now trusts whatever conclusion check.json carries on a run that produced no verdict, and the new comment step posts a fresh comment per run with no dedupe.

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

Blockers

• None at the file/PR level.

Non-blocking

• The new step's behaviour cannot be exercised without a live no-verdict run (the PR says as much). If it is cheap to do, a one-off run against a deliberately broken pin before merging would confirm the != 'true' condition and the gh api POST path, since a typo here only shows up on the day something else is already broken.
• 3 suggestion(s)/nit(s) flagged inline on specific lines.

[github-actions]

[suggestion] Dropping the verdict gate here makes the step publish whatever conclusion the file holds, not only the no verdict failure it is written for. If the driver writes check.json but no (or an empty) verdict.md — a crash between the two writes, a truncated write, a future driver that emits the check first — this step posts a check named review with conclusion success against the reviewed commit, satisfying a required check, while the new step at the end of the job posts "This review did not complete" for the same run. That leans on an unstated cross-repo invariant (a no-verdict run always writes conclusion: failure), which is the same class of risk the position step below explicitly refuses to take on.

Elsewhere the safe conclusion is forced rather than read from the artifact: ai-review.yml defaults an unreadable verdict to failure, and the "Post the verdict" failure path posts conclusion=failure explicitly. A cheap parity fix is to add VERDICT_PRODUCED: ${{ steps.drive.outputs.verdict_produced }} to env and derive the conclusion as failure whenever it is not true, so a run that produced nothing can never publish a passing gate.

[bdchatham]

All three taken, in e1e38b8.

The check step trusting the artifact's conclusion. This is the sharpest of the three and it was a real fail-open. The step now derives the conclusion itself: VERDICT_PRODUCED rides in env, and a run that produced no verdict publishes failure whatever the file says. The file is written by a separate process across a repository boundary, so a crash between two writes could otherwise satisfy a required check while the step at the end of the job reported the review did not complete. The artifact still supplies the title and the summary; the one thing that gates a merge is decided here.

The continue-on-error claim. You are right that it did not hold, and softening it was the weaker of the two fixes you offered. The drive step now forces rc=5 — the driver's own code for this — when a review-mode run produced no verdict and the driver exited 0. So the claim is true rather than reworded, and a review that produced nothing can no longer leave a green job with an annotation as its only record.

The accumulating notice. Upserted on the marker, which is what the marker was there for. The verdict comment accumulates on purpose, because an author compares this review against the last one; a "did not complete" notice carries no such history, and your outage-plus-synchronize case is the one that made the difference — that path collects one identical copy per push.

The match is startswith on the marker and .user.type == "Bot", so a person quoting the marker cannot take the notice over. The marker reaches jq through $ENV rather than shell interpolation, following the file's own rule about jq command lines. Proven against the three cases with a real jq: the bot comment with the marker at the start selects, a human comment with the same marker does not, a bot comment with the marker mid-body does not.

The read is guarded and fails open: an unreadable listing leaves the id empty and this posts, which costs a duplicate notice rather than the notice itself.

Verification

python3 -c "yaml.safe_load(...)"   parses
actionlint base   4 SC2102
actionlint head   4 SC2102        unchanged
jq filter         3/3 cases correct

One # shellcheck disable=SC2016 with its reason, on the jq line — shellcheck reads $ENV as a shell expansion in single quotes, which is exactly what the single quotes are protecting it from.

On the live no-verdict run. Agreed that a typo here only surfaces on the day something else is already broken. It is not cheap before merging: producing one needs a driver pin that writes check.json without verdict.md, and both callers pin uses: by sha, so the workflow under test is not the one they run. The bump that carries v0.14.0 to a caller is where this gets exercised, and I will drive a deliberate no-verdict run there rather than assert the condition works.

[github-actions]

[nit] "A no-verdict run has already failed the job at the drive step" is not always true. The drive step ends in exit "$rc", so a driver that exits 0 without writing verdict.md only produces ::error::review produced no verdict (exit 0) — an annotation, which fails neither the step nor the job. In that path, if the POST below also fails, continue-on-error: true leaves a green job with a log annotation as the only record, which is the outcome this step exists to prevent. Either soften the claim, or make the drive step exit non-zero when a review mode run reaches no verdict.

[github-actions]

[suggestion] This POSTs a new comment on every no-verdict run, and MARKER is never read back by anything. For the verdict comment the accumulation is deliberate and justified (earlier verdicts are the record an author compares against), but a "did not complete" notice carries no such history — during an outage, or on a caller wired to the automatic pull_request path, an active PR collects one identical comment per push. Since the marker is already defined, consider upserting on it (search issue comments for MARKER, PATCH if found) so repeated infrastructure failures leave one notice rather than a pile. If the accumulation is intended, a line saying so would match how the verdict step documents the same choice.

[cursor]

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 fdeb800. Configure here.}

[github-actions]

The change is well-reasoned and correctly scoped: dropping the verdict gate on the check-run publish while keeping it on the position step is the right split, the new comment step is a true complement of Post the verdict, and every post-drive step already runs on !cancelled() so forcing rc=5 skips no publisher. Three non-blocking gaps: Answer the request still keys off the check conclusion and will now thumb-down a no-verdict run, and the new notice is edited in place with no withdrawal when a later run succeeds.

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

Blockers

• None at the file/PR level.

Non-blocking

• The new step's ::error:: line says "the review reached no verdict" on both paths, including the run that died before the driver — where the body itself is careful to say the review never started. Minor, but that log line is the record that survives when the comment fails to post.
• Verified the mechanical claims that matter: every step after drive carries !cancelled(), so forcing rc=5 skips no publisher; drive itself has no if:, so a pre-driver failure skips it and leaves verdict_produced unset, which the new step's != 'true' correctly catches; and CHECK is empty on that path, so Publish the review check run still exits 0 without a gate. The startswith marker also cannot collide with the verdict comment's shorter <!-- seidroid-review -->.
• 3 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] Publish the review check run reads jq -r .conclusion "$CHECK" and jq -r .title with a bare field rather than // empty (unlike State the review's position, which uses // empty and explains why). A check file missing conclusion renders the string "null" and the gh api POST is rejected with a 422 that names nothing useful. This PR widens when the step runs but does not introduce the pattern.

[github-actions]

[suggestion] "Neither takes a position on the pull request" doesn't hold once the driver writes a failure check on a no-verdict run.

Answer the request (line ~1294) derives its reaction purely from the check file:

case "$conclusion" in
  failure) reaction="-1"; stale="+1 eyes" ;;

Its own comment documents the assumption it is built on — "the arm that reaches no verdict withdraws them too", i.e. the *) arm — and that arm is reached only because the check file is absent today. Under v0.14.0 the file is present with conclusion: failure, so the failure arm wins and a run that reviewed nothing leaves 👎 on the requester's comment: the same signal a review that found blockers gives, and the direct opposite of the new comment's "This review did not complete."

That step wasn't touched here, but this PR is what makes the check file present on the no-verdict path, so the reasoning about which steps keep verdict_produced needs to cover it too. Either gate the reaction on verdict_produced, or key the case on the check title — which is what the rest of this PR says is the field that distinguishes the two.

[bdchatham]

All four taken, in 329762c. One correction first.

My earlier reply claimed the fail-closed conclusion had landed. It had not. The script that made the edit exited on a later failed match before writing the file, so two of the three changes I described were never in the diff — the commit message asserted them too. VERDICT_PRODUCED is in the step's env now, and the bare jq -r .conclusion interpolation is gone. Verified by grep rather than by the script's own report this time.

The 👎 on a no-verdict run. This is the one that mattered, and it is exactly the shape of defect this PR exists to prevent — making the check file present on a path where downstream steps were written assuming it is absent. Answer the request now reads verdict_produced before it reads the conclusion, so a run that reviewed nothing falls to the *) arm and clears every reaction it left. Your alternative — keying the case on the title — is also correct, but the gate the workflow already computed is a cheaper truth than a string comparison against a title the driver owns.

Editing in place. You are right, and you cited the file's own reasoning back at me, which is the strongest form of the argument. The notice is deleted and reposted now: still one notice, lands at the bottom, notifies, and carries this run's url so each occurrence is distinguishable. That also removes the failure you spotted — the missing POST fallback after a failed PATCH. The deletion is tolerated and the post runs either way, so a failed deletion costs a duplicate rather than the notice.

Nothing withdraws the notice. Added to Post the verdict, on the success path, symmetric with how the notice step reaches back for its own. Under its own env key: that step already binds MARKER to the verdict's own marker, and one env block cannot carry two keys of one name — I introduced exactly that collision and caught it before pushing, which would have re-prefixed every verdict comment with the no-verdict marker.

The ::error:: line. Fixed. started records which of the two paths reached it, and the log line says which.

On the pre-existing jq -r .conclusion. Taken in the same change, since I was rewriting those lines: the conclusion reads // empty and a check file carrying none says so and fails, rather than posting the string "null" into a 422 that names nothing. Title and summary fall back rather than fail — neither gates a merge.

Verification

actionlint base   4 SC2102
actionlint head   4 SC2102        unchanged
yaml.safe_load                     parses
env key audit     Post the verdict: 1 MARKER, 1 NO_VERDICT_MARKER, 0 $ENV.MARKER uses

Still not exercised on a runner: no live no-verdict run yet, for the reason in my earlier reply.

[github-actions]

[suggestion] Editing the notice in place reintroduces the hazard Post the verdict documents at length (line ~1526):

Editing leaves the comment at its ORIGINAL position in the thread and notifies nobody, so a re-review became close to invisible: it did not move, it did not notify [...] indistinguishable from a review that never ran, and that cost real diagnosis time.

On a second consecutive no-verdict run the PATCH updates a comment that may be far up the thread and notifies no one, so the person who just typed @seidroid review gets a red job and — because Answer the request's reaction is the only other signal — nothing new on the pull request. That is the exact failure mode this PR exists to close, on the second occurrence rather than the first.

The stated reason for upserting is an outage on a caller wired to every push. Refreshing the run URL is what makes each occurrence distinguishable, so consider deleting the previous notice and posting a fresh one rather than PATCHing: still one notice, but it lands at the bottom and notifies.

Separately, if the PATCH fails there is no POST fallback — existing non-empty plus a failed PATCH falls straight through to the error log, so nothing reaches the pull request at all.

[github-actions]

[suggestion] Nothing withdraws this notice. "One notice per pull request" holds within the no-verdict path, but once a later run does reach a verdict, Post the verdict appends a new comment and leaves the standing "This review did not complete." notice untouched — permanently asserting a state that is no longer true, and linking a run that is no longer the latest.

The check run self-heals here (a new review check on the new commit supersedes the failing one); the comment does not. Consider having Post the verdict delete or amend <!-- seidroid-review:no-verdict --> when verdict_produced == 'true', which is symmetric with how this step already reaches back for its own prior notice.

[github-actions]

Drops the verdict_produced gate on the check-run publish and adds a complementary step that comments when a review reaches no verdict; the condition pairing is sound, the merge-gate-clearing step correctly keeps its gate, and no blocking issues were found. A few robustness gaps remain around unparseable check files, notice cleanup, and comment size.

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

Blockers

• None at the file/PR level.

Non-blocking

• .github/workflows/seidroid-review.yml:871 — forcing rc=5 flips a driver that exits 0 without writing verdict.md from a green job to a red one that also posts a public "This review did not complete" notice. That is the intended behaviour for a crash, but worth confirming the driver has no legitimate exit-0-with-no-verdict path (a deliberately skipped review, a PR closed mid-run); there is no exit-code contract recorded in this repo to check against.
• The new step's condition and shell are unverified against a live runner (the PR states this). Since Report a review that reached no verdict is the only record left on a run that dies before the driver, it is worth confirming on the first real no-verdict run that the step fires and that steps.drive.outputs.verdict_produced is empty-not-true in the skipped-step case, before relying on it as the sole signal.
• 3 suggestion(s)/nit(s) flagged inline on specific lines.

[github-actions]

[suggestion] The // "review" and // "" defaults only cover a missing field, not an unreadable file — and the no-verdict path this PR adds is now the one path that can reach the POST with an unparseable $CHECK.

When $CHECK is non-empty but not valid JSON: conclusion comes back empty (line 1034 is defended with || true), then line 1035 forces it to failure, so the -z guard on line 1038 never fires. Execution reaches here, jq -r '.title // "review"' exits non-zero and prints nothing, and the command substitution yields "" — so the check posts with output[title]="". That is either rejected by the API (422, leaving no check at all on exactly the run this PR exists to surface) or published with a blank title, which defeats the "read the title, not the conclusion, to tell the two apart" contract stated at line 1001.

Reading title/summary into shell variables once, with the same || true guard and a shell-side default, closes it and also avoids re-running jq on line 1061:

title="$(jq -r '.title // empty' "$CHECK" || true)"; : "${title:=review}"
summary="$(jq -r '.summary // empty' "$CHECK" || true)"

[github-actions]

[suggestion] tail -n 1 deletes only the newest matching notice, so the "exactly one is ever on the pull request" invariant stated just above never re-converges once it is broken. The comment on line 1740 accepts that a failed deletion costs a duplicate, but that duplicate is then permanent: the next no-verdict run again deletes only the newest, and each subsequent run adds one more. Looping over every id instead makes the invariant self-healing.

Separately, 2>/dev/null ... || true makes a failed listing (API error, or a jq expression the installed gh cannot compile — $ENV needs gojq's environ loader) indistinguishable from "no prior notice", with no annotation at all. Every other failure in this file states itself; this one is silent. Capturing the exit status and emitting a ::warning:: on a listing failure would match the surrounding style.

The sibling block at line 1593 has the same two properties.

[github-actions]

[nit] The step comment above says "No byte-cap arithmetic … what it appends is a bounded summary the driver clipped" — but it is bounded to the check-run summary limit (65535 chars), not to the issue-comment limit minus this body's own overhead (marker + header + URL, ~250 bytes). A driver that writes a long reason here (a stack trace, a truncated transcript) produces a body over 65536 and the POST is rejected, so the notice this PR exists to publish is lost to the log on the very run where it matters most.

reason="$(printf '%s' "$reason" | head -c 60000)" after line 1712 is enough, given the rest is fixed-size.

[bdchatham]

Checked the exit-0-with-no-verdict question against the driver source rather than leaving it open.

The path is real, and it is not a legitimate skip. runReview returns nil — process exit 0 — whenever result.ExitCode == ExitOK (cmd/sei-agent-driver/main.go:314-319). result.ExitCode is ExitOK when the turn produced a reply that passed w.Complete() (internal/driver/driver.go:130-137). A reply can pass that and still carry no readable decision block, in which case report writes no verdict.md and the driver exits 0. That is the case rc=5 catches, and it is a review nobody can read — exactly what should hold a merge.

There is no deliberate-skip mode to break. close is handled by its own branch above the one that forces rc=5, and a closed pull request never reaches the driver. So the flip has no legitimate green run to turn red.

You are right that no exit-code contract is recorded in this repo. The codes live in sei-agent-driver/internal/driver/errors.go, and PLT-1165 is where the workflow gets a way to assert the driver it installed matches what this file expects.

On the runner confirmation: agreed, and it is the first thing I will do on the pin bump that carries v0.14.0 to a caller. verdict_produced is unset on the skipped-step path and the condition is != 'true', so empty passes it — but that is reading, not running, and I will say which once it has run.