feat(seidroid-review): let a pull request ask for nits - #94
Conversation
PR SummaryLow Risk Overview The workflow adds a preflight step that reads PR labels (outside the driver step so no GitHub token reaches the agent) and sets Without the label, nits are still surfaced in the verdict/check summary as non-blocking prose, not dropped. With the label, nits post on the relevant lines and can supersede prior threads. Label read failures fail open (review runs with nits off); relabeling alone does not re-trigger a review. Reviewed by Cursor Bugbot for commit cd3a736. Bugbot is set up for automated code reviews on this repo. Configure here. |
6dbbe16 to
eaec656
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-guarded wiring of a per-PR ai: nitpick label to the driver's --include-nits flag: the read is fail-open, the token stays out of the drive step's env, and the label reaches jq via $ENV rather than string splicing. Two non-blocking points: the new flag was not added to the driver contract check that enumerates every flag this workflow passes, and the step repeats a GET /pulls/{n} the step directly above it already makes with the same token.
Findings: 0 blocking | 1 non-blocking | 0 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The new opt-in path has no automated coverage in the repo; the PR body describes a local harness that extracts the two step scripts and asserts the
ifexpression andINCLUDE_NITSwiring, but nothing in.github/runs it, so a future rebase that breaks the wiring fails silently rather than in CI. Worth landing that harness alongside the change if it is meant to hold.
Comments that couldn't be anchored to the diff
.github/workflows/seidroid-review.yml:1459-- [suggestion]--include-nitsis not added to the driver contract check's flag list (line 1146-1147). That loop is documented as covering "every long flag ... which this workflow passes", and--conversation-contextis in it despite also being passed conditionally — so the convention is that a conditionally-passed flag still belongs there.
Impact is limited today: review mode enforces MIN_DRIVER_VERSION = v0.14.0, which has the flag, so the gap only bites a newer driver that renames or drops --include-nits — precisely the "too new, or simply not this driver" case the check exists to catch. In that case a nit-labelled PR would fail inside the drive step, after the credential is held and the session is open, rather than at install time.
Adding --include-nits to the for flag in ... list closes it.
.github/workflows/seidroid-review.yml:1380-- [suggestion] This is a secondGET /repos/{o}/{r}/pulls/{n}against the same object theRecord the commit under reviewstep fetched immediately above (line 1345), with the identical token expression, identicalREPO/PR, and anifthat differs only by the label check. The PR description justifies not sharing the guard's read (different identity, earlyexit 0on deny), but that reasoning does not apply to the adjacent step — neither constraint holds there.
One gh api capturing the JSON once and emitting both sha and include_nits would drop the duplicate call. The trade-off is that a failed read would then cost both signals instead of one, which is worth stating in the comment either way — right now the file makes the same request twice without noting that it does.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 eaec656. Configure here.
The driver takes --include-nits and its prompt reads it on both settings; the workflow passed it on neither, so every review ran with nits off and no author could ask otherwise. The ask is a label on the reviewed pull request, `nitpick-label`, matching ai-review.yml's input of the same name and default. The person who wants a polish pass is the author of one pull request, not the person who wired the workflow, and a boolean input cannot be set per pull request. The read is its own step in the review job rather than a line in the guard. The guard reads the same object for the skip label under the App identity alone, which cannot answer a caller that configured no App; sharing that call would mean restructuring a fail-open/fail-closed admission check for a signal that decides nothing about admission. It costs one read of the pull request. Not in the drive step itself: the read needs a GitHub token, and that step's env reaches the driver process. Off is unchanged, and off means redirected. Without the label the driver sends a nit-grade finding to the non-blocking bucket, where the verdict comment and the check summary carry it as prose and no thread opens on the line. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The install step checks `review --help` for every long flag this file passes and refuses a driver missing one, before a session opens or quota is spent. --include-nits was not on that list, so a driver that dropped or renamed it passed the check and failed later inside `Drive session + collect verdict`, after install had already admitted it. The label read now takes `.labels[]?.name`, the shape the guard's own label check uses. A payload carrying no labels key answers false rather than failing the read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
eaec656 to
cd3a736
Compare
Right on the behaviour, and worth splitting the term: the review fails open, the Two changes since the commit you read ( |
There was a problem hiding this comment.
Wires the previously unreachable --include-nits driver flag to a per-PR ai: nitpick label, with a dedicated read step and the flag added to the install contract check. Logic, failure handling (continue-on-error, unset output → nits off), token placement and injection safety ($ENV in jq, env-routed values) all check out; only a minor redundancy in the API read.
Findings: 0 blocking | 2 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Behaviour of
--include-nitsitself lives in the driver (sei-agent-driver), so the input's claims about where a nit lands and what it may supersede can't be verified from this repo. Worth a one-line pointer to the driver docs if that contract ever drifts, but the version floor (v0.15.0) plus the contract check covers the flag's existence. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| # the review that was asked for, carrying one fewer kind of finding. | ||
| read_ok=true | ||
| # shellcheck disable=SC2016 # $ENV is jq's own, and jq reads it | ||
| carries="$(gh api "repos/$REPO/pulls/$PR" \ |
There was a problem hiding this comment.
[suggestion] This is the same GET /repos/{o}/{r}/pulls/{n} the immediately preceding Record the commit under review step already makes (line 1346), under the same token, the same REPO/PR env and a compatible if/continue-on-error. The PR description explains why the read isn't shared with the guard or folded into the drive step, but not why it isn't folded into this adjacent step, which would cost one API call instead of two and one --jq change.
The counter-argument is failure isolation — merged, a single failed read loses both sha and the nit setting — so this is fine as-is if that's the intent; it just isn't stated anywhere the way the other two placement decisions are.
1 participant
An author can now ask for a polish pass.
seidroid-review.ymlnever passed--include-nits, soIncludeNitswas false on every review and the driver's nitsetting was unreachable from a caller. This wires it to a label on the reviewed
pull request, and adds the flag to the install step's contract check.
Label, not a boolean input
The person who wants nits is the author of one pull request. A boolean input keys
off the caller's configuration, so it turns nits on for every pull request in the
repository or for none. A label is set per pull request, by the author, without a
workflow edit.
nitpick-labelmatchesai-review.yml's input of the same name and itsai: nitpickdefault (ai-review.yml:66-70), so a repository running both toolsadds one label rather than two. Its two consumers there are the prompt
(
ai-review.yml:783) and the poster (ai-review.yml:799, 838, 896), both fedfrom one label read in the preflight resolve step (
ai-review.yml:268).Empty disables the check, which is
skip-review-label's semantics in this filerather than
ai-review.yml's.One semantic does not transfer.
ai-review.ymlre-runs on alabeledevent whenthe changed label is the nitpick one. This workflow reviews a pull request once
and states that a relabel earns no second review, so adding the label starts
nothing. The author labels the pull request and comments
@seidroid review. Theinput says so.
Where the read goes, and what it costs
Its own step in the review job,
Read the nit setting from the pull request,immediately before the driver invocation. It costs one
GET /repos/{o}/{r}/pulls/{n}on the review path only.
Not in the guard's
Admit the request. The guard's skip-label read runs underGH_TOKEN, which is the App token with no fallback; a caller that configured noApp would never be able to opt in. Answering that caller needs
GATE_TOKEN, whichis a separate
gh apicall whichever job it lives in — so sharing the guard's callwould mean restructuring a fail-open/fail-closed admission check for a signal that
decides nothing about admission. The guard also
denys byexit 0mid-step, so anoutput added after the label check is not written on a denied path.
Not inside the drive step either, which is the tighter constraint. That step
deliberately carries no GitHub token, and its env reaches the driver process. A
GH_TOKENthere would hand the reviewing agent a GitHub credential.The read uses
any(.labels[]?.name; . == $ENV.NITPICK_LABEL), the shape #93 givesthe guard's own label check. It answers a failed read differently, and on purpose:
the skip label withholds work, so refusing on a signal nobody could read is the
safe answer there; this label asks for advice, so refusing would spend the review
to protect the polish pass. A failed read warns and leaves nits off. The comment
says so beside the code.
The install contract check
The install step reads
review --helpand refuses a driver missing any long flagthis file passes, before a session opens or quota is spent.
--include-nitsis nowon that list. Without it a driver that dropped or renamed the flag would pass the
check and fail inside
Drive session + collect verdict, after install had alreadyadmitted it — which is the failure the check exists to move earlier.
The list is confirmed complete against the argv the drive step actually builds,
not against the list as written; see report 3 below.
What turning nits on changes on the pull request
Off does not mean dropped. Read against
sei-agent-driveratv0.15.0, which isboth the
driver-versiondefault andMIN_DRIVER_VERSIONafter #95:nitRulesends it tonon_blockers: prose in the verdict comment and in the check run's Non-blocking section, no thread on the codenit: a comment thread on the linePlaceableFindings(findings.go:128), the counts (findings.go:373viacountFindings), and the check summary, which renders only the line-less bucketsSo the label chooses where a nit lands, not whether the review makes one. The
prompt states the current setting on both settings and says it replaces an earlier
one (
prompt.go:527-548) — load-bearing here, because the session outlives the runand a first turn told to leave nits out still holds that instruction.
Verification
Nothing ran on a GitHub runner. Three step scripts —
Install the review driver,Read the nit settingandDrive session + collect verdict— were extracted fromthe shipped file with a YAML parser and run under
bashwith stubs. The harnessasserts each step's
ifand theINCLUDE_NITSwiring against the file, so arebase that changes one fails the harness rather than passing it. The
ghstubruns the shipped
--jqfilter through realjq; thegostub serves a driverwhose reported version and
review --helpflag set the case controls.1. driver argv
every input setexists so the union of flags below is the whole surface. Its argv:Close mode:
review sei-protocol/uci 42 --close.2. install contract check
The stub help gives half the flags a cobra shorthand (
-x, --out string), so thecheck is exercised against the shape its own comment says it must tolerate.
3. contract list against real argv
Parsed out of the shipped install script and compared with the union of long flags
the drive step actually built across all ten cases:
Both directions are assertions, so wiring a flag without listing it, or listing one
the workflow never passes, fails the harness.
The real
sei-agent-driver@v0.15.0was installed from the proxy and itsreview --helpnames exactly--check-out --close --conversation-context --extra-instructions --findings-out --guidelines-file --help --include-nits --out --trigger-id— the contract list plus--help.The
--jqfilter was also run throughgh's own engine (github.com/cli/go-gh/v2/pkg/jqv2.16.0): label present
true, absentfalse, empty arrayfalse, nolabelskey
false,ai: nitpickyfalse,AI: Nitpickfalse.actionlint1.7.12 on the same path with the same invocation: base30f5c09gives 4 findings, all
SC2102:info; this branch gives 4, allSC2102:info.shellcheck -S infoon all three extracted scripts: clean. The file parses underyaml.safe_load; 19workflow_callinputs.Not verified: any live run, and the flag's effect on a real model turn.
🤖 Generated with Claude Code