fix(seidroid-review): withdraw the eyes when the review answers - #84
Conversation
PR SummaryLow Risk Overview Acknowledge the trigger now runs only when In Answer the request, the bot’s stale-reaction cleanup includes Reviewed by Cursor Bugbot for commit f37d6c6. Bugbot is set up for automated code reviews on this repo. Configure here. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The core change is correct: all three case arms now include eyes in stale, the acknowledgement and the withdrawal both react as github-actions[bot] via github.token, the select(.user.login == $me) scope still spares human reactions, and the case \" $stale \" in *\" $content \"* word match is unambiguous across GitHub's reaction contents. The gap is that the acknowledgement step carries no mode gate while the withdrawal step does, so a comment-triggered close still strands the eyes — contradicting the new comment that says otherwise.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The
!cancelled()gate onAnswer the requestleaves one more path where the eyes survives: when a newer@seidroid reviewcancels an in-flight run (cancel-in-progress: true), the cancelled run skips the step, and the superseding run replays the new comment id — so the older trigger comment keeps its 👀 permanently, reading as still-running. If that path is meant to be out of scope, the new comment at.github/workflows/seidroid-review.yml:624("on every path it takes") is the place to say so. Answer the requestruns beforePost the verdict(line 1346), so the eyes is withdrawn and the thumb applied while the verdict comment does not yet exist. IfPost the verdictthen fails (it iscontinue-on-error: true), the requester is left with a thumb, no eyes, and no verdict. Placing this step after the posting step would make the withdrawal coincide with the answer actually landing.- 1 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] The guard's
parsestep writes acommand=close/command=reviewoutput (.github/workflows/seidroid-review.yml:384-386) but the guard never exposes it as a job output, and nothing outside the guard reads it beyond theadmitstep's skip-label bypass. The routing decision it encodes is duplicated in the caller'smodeinput, so the two can disagree with nothing to catch it.
| # the same comment returns the existing reaction rather than adding a | ||
| # second one, so a retry needs no cleanup. | ||
| # | ||
| # `Answer the request` withdraws this reaction, on every path it takes. A |
There was a problem hiding this comment.
[suggestion] This claim does not hold for a comment-triggered close. Acknowledge the trigger is gated only on needs.guard.outputs.comment_id != '' (line 612), while Answer the request additionally requires inputs.mode == 'review' (line 1253). The guard explicitly supports issue_comment + mode: close — parse sets command=close for @seidroid review close (line 384) and still emits comment_id (line 422), and the review job's condition admits github.event_name == 'issue_comment' at any mode (line 542), with lines 526-528 stating that a close asked for in a comment goes through the guard on purpose.
So on a teardown request the acknowledgement posts 👀 and nothing ever withdraws it — the exact permanent "still in progress" state this PR exists to remove, just moved to the close path. Either gate the acknowledgement on inputs.mode == 'review' so a close never earns eyes it cannot lose, or drop the mode gate from Answer the request so the close path clears them too. As written the comment reads as a guarantee that is not in the file.
A finished review read as a review still in progress. `Acknowledge the trigger` reacts eyes on the triggering comment, and no step took it off, so the acknowledgement stayed on the request for good. `Answer the request` now names eyes in every stale set, beside the thumb each conclusion replaces: `+1 eyes` on failure, `-1 eyes` on success, and `+1 -1 eyes` where the conclusion earns no reaction. The listing already returns the content beside the id for this bot's own reactions. The clearing loop therefore deletes the eyes with no extra call, and it widens no scope. The step holds every property it had. It stays off `verdict_produced`, so a run that reaches no verdict still clears what an earlier attempt left. An absent check file still falls to the clearing arm rather than returning. The delete stays scoped to `github-actions[bot]`, so a human's reaction survives. The step still runs no pipeline, and it still skips the POST when the conclusion earns none. Measured against a gh stub over 25 runs: five starting states by five conclusion inputs. The eyes goes in every run where this bot left one. A human's `-1` and a human's eyes survive all 25. Refs PLT-1144
`Acknowledge the trigger` was gated only on a comment id, while `Answer the request` also requires a review turn. So `@seidroid review close` posted the eyes and nothing withdrew them: the requester reads a teardown that is still running, forever. The two steps now name the same two facts. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
7b275f6 to
f37d6c6
Compare
|
Rebased onto The gap: The two steps now name the same two facts. Every eyes this workflow posts is a review turn, and every review turn reaches the step that withdraws it. |
There was a problem hiding this comment.
The fix is correct for the paths it covers: eyes is added to all three stale sets, the withdrawal stays scoped to github-actions[bot], and gating the acknowledgement on mode == 'review' keeps it symmetric with the step that clears it. Three non-blocking gaps remain, around cancellation, the close path, and delete/post ordering.
Findings: 0 blocking | 4 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The PR description's behaviour table covers review mode only; the close path's new silence (no reaction at all) isn't characterised there, and it is the one user-visible behaviour change beyond the eyes withdrawal.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
| # request to tear a session down earns no answer. | ||
| # No verdict_produced gate: a re-run replays the trigger comment id, so a run | ||
| # reaching no verdict still has to clear a thumb an earlier attempt left there. | ||
| # The condition names three facts: a review turn, not cancelled, and a comment |
There was a problem hiding this comment.
[suggestion] !cancelled() still strands the eyes, so "withdraws them ... on every path it takes" isn't quite true. Cancellation is a routine path here, not an exotic one: this job sets cancel-in-progress: true keyed on (mode, repo, PR), so a second @seidroid review on the same pull request cancels the first run. The cancelled run skips this step, and because the newer run replays a different trigger comment id, nothing ever clears the eyes off the first comment — it reads as permanently in progress, which is the exact defect this PR fixes elsewhere.
ai-review.yml, cited in the description as the workflow that removes it on every path, uses always() on its complete_review_reaction job for this reason; always() steps do run during a job's cancellation grace period. Switching to always() here needs no other change: a cancelled run has no readable check file, so it falls into the *) arm, clears +1 -1 eyes and posts nothing, which is the right answer for a superseded run.
Related: the new comment on Acknowledge the trigger says this step "runs on the same two facts" — it runs on three, and the third is the one that opens this gap.
| # A review turn, and a comment to react to. `Answer the request` withdraws this | ||
| # reaction and it runs on the same two facts, so a mode it skips must not react | ||
| # here: a close would leave eyes that nothing clears. | ||
| if: ${{ inputs.mode == 'review' && needs.guard.outputs.comment_id != '' }} |
There was a problem hiding this comment.
[suggestion] Adding inputs.mode == 'review' is the right call for symmetry, but it leaves a comment-triggered close with no signal at all. @seidroid review close parses to command=close with comment_id set (guard, lines 390-400), the review job runs, and every step that touches the pull request — this one, Answer the request, Post the verdict, State the review's position — is now gated on mode == 'review'. So a person asking for teardown gets nothing back: no eyes, no thumb, no comment, while the drive step reclaims the sandbox for however long that takes. Previously they at least got a receipt.
If the close path is meant to stay silent, that's a defensible choice, but it's worth stating in the comment here so a later reader doesn't read the omission as an oversight. Otherwise, a close-mode step that reacts and then clears on completion (or simply posts a terminal reaction once teardown finishes) would restore the acknowledgement without reintroducing the uncleared-eyes problem.
|
|
||
| # Withdraw this bot's stale thumbs first. A reaction is not a toggle and a | ||
| # re-run replays the same comment id, so a comment would otherwise wear a | ||
| # Withdraw this bot's stale reactions first: the eyes that acknowledged the |
There was a problem hiding this comment.
[suggestion] Withdrawing before posting means a failed POST — which the step deliberately treats as non-fatal — leaves the trigger comment wearing nothing: the eyes are gone and the thumb never arrived, so a finished review reads as one that was never seen. Before this PR the same failure left the eyes standing, which at least read as in-progress.
Posting first and withdrawing second avoids that: the POST is idempotent per (identity, content), so a re-run is safe, and the worst case becomes a comment briefly wearing both eyes and a thumb (or, if a DELETE then fails, permanently — but with the answer visible, which is the part that matters). The reaction="" arm is unaffected since it has nothing to post.
…the dismissal (#88) The step that records the review's position required the App identity, so a repository with no `SEIDROID_APP_ID` got a verdict comment and never appeared in the reviewers list. It now falls back to `github.token`, which is what every sibling publisher in this file already does. ## What changed - `State the review's position on the pull request` drops `steps.identity.outputs.token != ''` from its condition and reads `steps.identity.outputs.token || github.token`. The App still authors the position wherever a caller configured one. - The position post gains a `COMMENT` rung, mirroring `.github/workflows/ai-review.yml:983-986`. When the API refuses `APPROVE` or `REQUEST_CHANGES`, the step retries the same body as `COMMENT` and warns. - Two comments now state the present: the position step names what the fallback identity costs, and the `Answer the request` step names why it stands apart from the position step. The withdrawal, its interlocks and its loud failure on a stuck block are untouched. ## What a GITHUB_TOKEN review can and cannot do **Measured in this repository.** `github-actions[bot]` — the login `GITHUB_TOKEN` carries — has created `APPROVE` reviews here through `ai-review.yml`'s identical fallback: PRs #84, #83, #75, #74, #73, #72, #71, #70 and #61 all carry a `github-actions[bot]` `APPROVED` review whose body opens with `<!-- ai-combined-review -->`. It has also created `REQUEST_CHANGES` reviews and dismissed them: the timeline of #75 records two `review_dismissed` events with `actor=github-actions[bot]`, `state=changes_requested`, message `Superseded: latest AI review found no blocking issues.` So the create path and the dismissal path both work under this repository's settings. That the approvals land at all means sei-protocol allows Actions to approve a pull request. GitHub refuses the call outright when that setting is off, and none of these calls was refused. **Cannot.** A `COMMENT` review casts no vote. Where the rung fires, an `APPROVE` clears no approval requirement and a `REQUEST_CHANGES` blocks no merge. The check run remains the gate in both cases. ## The branch-protection question **Neither identity clears the review requirement on `sei-chain` today.** sei-chain PRs #4094 and #4100 each carry exactly one review — `seidroid[bot]` `APPROVED`, from the App — and GraphQL `reviewDecision` on both reads `REVIEW_REQUIRED`. sei-chain's `main` is protected and carries `.github/CODEOWNERS`. So a bot approval, App-authored or not, does not satisfy that gate now. The fallback costs nothing there that the App identity was buying. `uci` itself has no gate to satisfy. `main` reports `protected: false`, and the one ruleset (id 7955617, `~DEFAULT_BRANCH`) is `enforcement: disabled`. Were it enabled it would ask for `required_approving_review_count: 1`, `required_reviewers: []`, `require_code_owner_review: false`, `dismissal_restriction.enabled: false` — a rule that names no reviewer and restricts no dismissal. ## What I could not verify - **Whether a `github-actions[bot]` approval satisfies a required-approval rule.** `GET /repos/sei-protocol/uci/actions/permissions/workflow` and the org equivalent both returned 403 for my token, so I could not read `can_approve_pull_request_reviews` directly — the successful approvals above are the inference. No repository in reach has a merge gate that an Actions approval has ever been put to. - **sei-chain's protection detail.** `GET /repos/sei-protocol/sei-chain/branches/main/protection` returns 404 for my token. CODEOWNERS is the probable reason `reviewDecision` stays `REVIEW_REQUIRED`, not a measured one. - **A live run.** Nothing here ran on a runner. The evidence is the incumbent's history, the API state above and the local battery below. - **The `COMMENT` rung under a real refusal.** I never saw the API refuse a position, so the rung is exercised against a stub, not against GitHub. ## Two consequences worth naming 1. A repository with no App now runs the withdrawal where it previously skipped the whole step. A dismissal the API refuses fails the job, by the design this step already states. That is the intended loud failure, and it is new exposure for those repositories. 2. If the first post lands server-side but the client reports a failure, the `COMMENT` retry writes a second review. `ai-review.yml:968-991` carries the same hazard; the cost is one extra review, never a changed gate. ## Verification `actionlint` 1.7.12, against base `543f11f`: ``` base: 4 findings, exit 1 -> 4 SC2102:info new: 4 findings, exit 1 -> 4 SC2102:info rule set diff: identical ``` Both findings pre-date this change and sit in steps it does not touch. ``` $ python3 -c "import yaml; yaml.safe_load(open('.github/workflows/seidroid-review.yml'))" yaml ok $ shellcheck -s bash <the position step's script> (clean) ``` **Behaviour battery.** I extracted the step's script from the YAML at both revisions, put a stubbed `gh` on `PATH`, and ran twelve paths against each: event accepted, event refused, every event refused, approve off, blocking, blocking with the event refused, dismissals refused, review list unreadable, neutral with a blocker, neutral with nothing written down, neutral with no counts, and no check file. The base and the new step produce byte-identical output on every path but the two where the new `COMMENT` rung fires: ``` --- clean, approve on, event refused -::warning::could not record APPROVE ...; the verdict comment stands +::warning::... would not take APPROVE, so this review is recorded as a comment; + the check run carries the success conclusion --- blocking, request changes refused -::warning::could not record REQUEST_CHANGES ...; the verdict comment stands +::warning::... would not take REQUEST_CHANGES, so this review is recorded as a + comment; the check run carries the failure conclusion ``` Both loud failures still exit 1: the stuck dismissal and the unreadable review list. All four withdrawal interlocks are unchanged — a blocker beside a soft conclusion stops the withdrawal, a soft conclusion with zero counts stops it, absent counts let it proceed with a warning, and a `failure` conclusion exits before it. ## One thing the ticket got wrong The ticket's phrasing — "drop the identity requirement from the step's condition" — was written against a base that still carried the cross-repository target grammar. `bf507f3` removed that grammar, so the step now only ever posts to `github.repository` and dropping the requirement outright is safe. Against the older base it was not: `github.token` reaches no other repository, the withdrawal's review listing would have 404'd, and the step would have failed the job reporting a block it could not see. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Implements PLT-1144.
The job adds an
eyesreaction to the triggering comment and never removes it.The
stalesets were+1,-1,+1 -1—eyesappeared in none. A finishedreview therefore reads as still in progress, permanently, and on a conclusion
that earns no thumb there is no completion signal at all.
ai-review.ymlremoves it on every path.The change
Three code lines:
The rest is comment accuracy on lines this makes stale. Two of those corrections
are worth naming, because each described a guard as something other than what it
does — the class of defect five review rounds on this step kept finding:
not; it names a comment to answer. The next line already said there is no
verdict_producedgate, so the block contradicted itself, and a readertrusting the first sentence would add the gate back.
would strand. The eyes is the larger loss.
Behaviour, measured
The extracted step driven against a
ghstub that runs the workflow's own--jqfilter, so the filter is under test. 25 runs: five starting states × fiveconclusion inputs.
Every run exits 0. No human reaction is deleted in any of the 25.
Degraded paths all exit 0: a failed listing warns and still posts; a failed
delete warns per reaction and still posts; a failed post leaves the withdrawals
standing.
Why the table is not vacuous
The first harness was wrong and passed everything including the base — an
assertion anchored on
$against a log line with a trailing space. Corrected,then mutation-tested after committing:
a32defaeyesoff the success armeyesoff the*armselect(.user.login == $me)removedThe last two are the guards that must not break, and the harness proves it would
notice.
Verification
Out of scope, filed separately
A cancelled run leaves the eyes on.
!cancelled()skips this step when theconcurrency group cancels the run, so two
@seidroid reviewcomments in quicksuccession leave the first wearing eyes with no answer ever coming.
ai-review.ymlavoids this by clearing in a separate job underalways().Changing when a step runs on cancellation is past this ticket.
🤖 Generated with Claude Code