Skip to content

fix(seidroid-review): read the finding counts where the driver writes them - #74

Merged
bdchatham merged 1 commit into
feat/seidroid-reviewfrom
fix/read-the-counts-where-the-driver-writes-them
Sep 5, 2026
Merged

fix(seidroid-review): read the finding counts where the driver writes them#74
bdchatham merged 1 commit into
feat/seidroid-reviewfrom
fix/read-the-counts-where-the-driver-writes-them

Conversation

@bdchatham

Copy link
Copy Markdown
Contributor

The bug

check.json carries the counts nested under counts, beside a top-level
conclusion. The step read the three counts at the root.

$ jq -r '[(.blocking // "?"), (.non_blocking // "?"), (.pre_existing // "?"), (.conclusion // "?")] | @tsv' check.json
?	?	?	success
$ jq -r '[(.counts.blocking // "?"), (.counts.non_blocking // "?"), (.counts.pre_existing // "?"), (.conclusion // "?")] | @tsv' check.json
0	2	1	success

The real shape, rendered by running BuildCheckRun on a verdict with two
non-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

  1. The findings line never printed a number. is_count "?" is false, so the
    N blocking | M non-blocking terms were always omitted — which is the whole
    feature 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.
  2. The notice was wrong about the driver. It announced that the driver
    "predates check.json's blocking and non_blocking fields" against v0.11.0,
    which was cut specifically to add them.
  3. Both disagreement warnings were dead code. They are guarded by
    is_count "$blocking", so neither could fire. That is the only automated
    cross-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. .conclusion was already correct and is untouched.

The sentinel still works for a genuinely older driver: absent counts answer
null at the nested path too, so is_count rejects them and the shorter line
still prints.

Verification

Both jq invocations above were run against the driver's real output.
actionlint reports 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

… 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>
@cursor

cursor Bot commented Sep 5, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Workflow-only jq path fix for comment formatting and log warnings; no changes to review logic, credentials, or merge gating.

Overview
The Post the verdict step was parsing check.json for blocking/non-blocking/pre-existing totals at the root, but the driver nests them under counts. The jq expression now uses .counts.blocking, .counts.non_blocking, and .counts.pre_existing (top-level .conclusion is unchanged).

That mismatch silently broke the verdict comment’s Findings line (counts never passed is_count), triggered a misleading notice that the driver “predates” those fields, and kept the workflow’s comment vs check-run disagreement warnings from ever running. A short inline comment documents the correct shape so the root paths are not reintroduced.

Reviewed by Cursor Bugbot for commit 9a80f36. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bdchatham
bdchatham merged commit 68406ee into feat/seidroid-review Sep 5, 2026
6 checks passed
@bdchatham
bdchatham deleted the fix/read-the-counts-where-the-driver-writes-them branch September 5, 2026 16:23
bdchatham added a commit that referenced this pull request Sep 6, 2026
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant