fix(seidroid-review): take the review's position on the review that carries the verdict, not a second one

[bdchatham]

Summary

Since v0.0.24 each run already posts the verdict and inline findings as one COMMENT review. After that, "State the review's position" submits a second review, REQUEST_CHANGES or APPROVE, whose body is only boilerplate. That second review shows up as noise on every run (e.g. sei-protocol/sei-chain#4376 (review)). This PR moves the position onto the review that already carries the verdict.

new step  "Decide the review's position" (id: position), runs before place
          the event/approve/head-stability logic moves here unchanged
          outputs: event, note, conclusion, head_state

place:    batched review  event = carries_verdict && EVENT ? EVENT : COMMENT
          422 on a non-COMMENT event -> retry the same batch as COMMENT
          no batched review carried the verdict, EVENT is REQUEST_CHANGES/APPROVE,
          and the batch did not fail ambiguously
              -> verdict-only review (no line comments) with EVENT,
                 same 422 -> COMMENT fallback            (carry_position)
          output: position_posted = the event that landed (only for REQUEST_CHANGES/APPROVE)

position: CARRIED (= place.position_posted) set -> post nothing
          otherwise the old post_position path (e.g. a COMMENT position and its note);
          the withdrawal of an earlier block is unchanged

• A review that records REQUEST_CHANGES opens with VERDICT_MARKER. The guard's block check and the withdrawal's CHANGES_REQUESTED + marker selectors therefore find it, the same way they found the old position-only review.
• A findings review without the verdict (the driver wrote none) stays COMMENT. Its body has no marker, so a block recorded on it could never be withdrawn.
• A COMMENT position (a downgraded approval, or a withheld comment) still goes out from the position step with its note, so the reason stays on the PR.
• APPROVE still requires approve-on-success, the reviewed SHA, and a stable head. That logic moved into the new step as-is.

Validation: actionlint with shellcheck 0.10.0 gives the same infos as main. I also ran the decide, place, position and verdict scripts against a stubbed gh in these cases: block with inline findings, block with no findings, block with a finding off the diff, 422 on REQUEST_CHANGES, an ambiguous 502 on the batch, a downgraded approval, and approve on. Where the event is REQUEST_CHANGES/APPROVE and the batch did not fail ambiguously, each run produced exactly one PR review.

This goes out in one tag together with #114, which is already merged into this branch.

Link to Devin session: https://app.devin.ai/sessions/9e65f0b138c84a248927a0aac3c24060
Open in Devin Desktop: https://app.devin.ai/desktop/session/9e65f0b138c84a248927a0aac3c24060?variant=devin
Requested by: @bdchatham

[devin-ai-integration]

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

• Disable automatic comment, CI, and merge conflict monitoring

[cursor]

PR Summary

Medium Risk
Changes how merge-blocking APPROVE/REQUEST_CHANGES land on GitHub PRs; logic was relocated rather than rewritten, with fallbacks and withdrawal paths preserved.

Overview
Moves approve / request-changes onto the same PR review that already carries the verdict and inline findings, instead of posting a second boilerplate-only review afterward.

A new Decide the review's position step runs before placement and outputs event, note, conclusion, and head-stability state (unchanged rules: approve-on-success, recorded SHA, no approve if the head moved). Place findings uses that EVENT on the batched review when it carries the verdict, adds carry_position for verdict-only runs with no findings, retries as COMMENT on 422, and exposes position_posted. State the review's position skips post_position when placement already recorded the event; block withdrawal and fallback posting behavior stay as before.

REQUEST_CHANGES reviews still open with VERDICT_MARKER so guard and withdrawal selectors keep working.

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

[seidroid]

The PR moves the review's position onto the review that already carries the verdict. The decide logic is moved over unchanged, the 422→COMMENT fallback is kept, and the withdrawal and guard selectors still match, because a REQUEST_CHANGES review still opens with VERDICT_MARKER. Two non-blocking gaps remain: when approval is withheld, the note explaining why is no longer posted anywhere on the PR, and a batched review whose response is lost can still be followed by a second verdict review.

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

Blockers

• None at the file/PR level.

Non-blocking

• None at the file/PR level.

Comments that couldn't be anchored to the diff

• .github/workflows/seidroid-review.yml:3135 -- [suggestion] When CARRIED is set, $note (POSITION_NOTE) is thrown away. For most events that's fine, because the verdict body covers it. The downgraded-approval case is the exception: decision approve, approve-on-success on, and the head moved or the reviewed SHA was not recorded. The decide step sets event=COMMENT with a note explaining why no approval was given. Before this PR, that note went out as its own COMMENT review. Now the batched review (or carry_position) records COMMENT with only the verdict, which reads as clean, and the reason approval was withheld appears only in the Actions log. That is the same "nothing on the PR says why" failure the decide step's comments warn about. Suggestion: when EVENT is COMMENT and the decision was approve, append the note to the carried review body (the placement step already has review_id, or it could be added to review_body / verdict_review's body), or have the position step still post the note in that case.
• .github/workflows/seidroid-review.yml:2840 -- [suggestion] Ambiguous-write case. The batched POST (or its 422→COMMENT retry) can land on GitHub and then lose its response, leaving no status line. In that case landed stays false and the findings correctly go to the summary so the write isn't repeated. But summary_posted also stays false, so carry_position then POSTs another review with the full verdict and, possibly, REQUEST_CHANGES. The PR can end up with two verdict-carrying reviews and two blocks. This roughly matches the old behaviour (verdict comment plus a separate position review), so it's not a regression in count. Still, carry_position seems a natural place to skip its POST when the preceding batch failure was ambiguous (empty status) and not a clear 4xx, and let the position step's existing path handle it. (Also raised by Codex.)

[devin-ai-integration]

Both fixed in 97de1c6:

• A COMMENT position (a downgraded approval, or a withheld comment) no longer goes on the carried review. The position step posts it with its note, as it did before this PR. Only REQUEST_CHANGES/APPROVE ride on the carried review.
• When the batched POST fails ambiguously (no response, or a non-4xx status), carry_position now skips its POST. The verdict comment and the position step handle that case, as they did before this PR.

[cursor]

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

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

^{Reviewed by Cursor Bugbot for commit 97de1c6. Configure here.}

[cursor]

Stale head state used later

Medium Severity

The head-stability check now runs in Decide the review's position, and later steps reuse that head_state. A push during Place findings or the check publish leaves head_state as same, so this run can dismiss a standing block — or post APPROVE — after the head moved to a commit it never read.

Additional Locations (2)

• .github/workflows/seidroid-review.yml#L2951-L2956
• .github/workflows/seidroid-review.yml#L3102-L3108

 

^{Triggered by learned rule: seidroid-review: withdraw blocks on counts, not the word neutral}

^{Reviewed by Cursor Bugbot for commit 97de1c6. Configure here.}

[devin-ai-integration]

Valid. The position step now reads the head again before it withdraws. That fix went into a follow-up PR on branch devin/1790777271-seidroid-head-recheck, because this PR merged before it was pushed.

[cursor]

Ambiguous batch still posts second vote

Medium Severity

ambiguous stops carry_position when the batched REQUEST_CHANGES or APPROVE call gets a 5xx or empty status, because that write may have landed. position_posted stays empty, so the later position step still submits the same event as a second review.

Additional Locations (2)

• .github/workflows/seidroid-review.yml#L2272-L2276
• .github/workflows/seidroid-review.yml#L3007-L3010

 

^{Reviewed by Cursor Bugbot for commit 97de1c6. Configure here.}

[devin-ai-integration]

Valid. After an ambiguous failure, placement now looks up the review by body and commit. If that review landed, it is treated as landed, so no second review is posted. The fix is in a follow-up PR on branch devin/1790777271-seidroid-head-recheck, because this PR merged before it was pushed.