fix(seidroid-review): read the finding counts where the driver writes them - #74
Conversation
… them
check.json carries `counts: {blocking, non_blocking, placeable, pre_existing}`
beside a top-level `conclusion`. The step read the three counts at the root,
where they are always null:
$ jq -r '[(.blocking // "?"), (.non_blocking // "?"), (.pre_existing // "?"),
(.conclusion // "?")] | @TSV' check.json
? ? ? success
$ jq -r '[(.counts.blocking // "?"), ...] | @TSV' check.json
0 2 1 success
Three consequences, all silent. The findings line never printed a number. The
notice announced that the driver "predates check.json's blocking and
non_blocking fields" against a driver that has them. And both disagreement
warnings are guarded by `is_count "$blocking"`, so neither could ever fire --
the only automated cross-check between this comment and the check run was dead
code, which matters most now that a clean conclusion can post an approval.
The sentinel still does its job for a genuinely older driver: absent counts
answer null at the new path too.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
PR SummaryLow Risk Overview That mismatch silently broke the verdict comment’s Findings line (counts never passed Reviewed by Cursor Bugbot for commit 9a80f36. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Corrects the jq path for the driver's finding counts from the root to .counts, which is where check.json writes them; the fix is complete (this was the only such read), keeps .conclusion at the root consistent with the workflow's other two consumers of the file, and preserves the sentinel fallback so an older driver still publishes the shorter findings line.
Findings: 0 blocking | 0 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
…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>
The bug
check.jsoncarries the counts nested undercounts, beside a top-levelconclusion. The step read the three counts at the root.The real shape, rendered by running
BuildCheckRunon a verdict with twonon-blockers and one pre-existing suggestion:
{"conclusion":"success","title":"2 findings, 1 pre-existing issue","summary":"…", "counts":{"blocking":0,"non_blocking":2,"placeable":0,"pre_existing":1}}Three consequences, all silent
is_count "?"is false, so theN blocking | M non-blockingterms were always omitted — which is the wholefeature feat(seidroid-review): fail soft when the verdict cannot post, and state the finding counts #71 added and sei-load#97
was opened to pilot.
"predates check.json's blocking and non_blocking fields" against
v0.11.0,which was cut specifically to add them.
is_count "$blocking", so neither could fire. That is the only automatedcross-check between the verdict comment and the check run — and it was dark
exactly as a clean conclusion becomes able to post an approval.
Scope
Four characters of path, plus a comment recording why the root read was wrong
so nobody re-derives it.
.conclusionwas already correct and is untouched.The sentinel still works for a genuinely older driver: absent counts answer
nullat the nested path too, sois_countrejects them and the shorter linestill prints.
Verification
Both jq invocations above were run against the driver's real output.
actionlintreports the same 5 pre-existing findings as the base(lines 326/1001/1183); none from this change. YAML parses.
Found by a platform review of the pipeline; the driver-side companion is
sei-internal-skills#399.
🤖 Generated with Claude Code