fix(seidroid-review): gate the guard's admission on the caller's mode and identity - #93
Conversation
PR SummaryMedium Risk Overview
Skip-review label is evaluated with Docs in the workflow inputs/secrets and inline comments were expanded to describe these behaviors; the parse step no longer emits a Reviewed by Cursor Bugbot for commit 740f013. 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.
All three targeted fixes (mode-keying the skip-label check, reading labels under GATE_TOKEN with fail-closed semantics, and defaulting allowed-team) are implemented correctly, and dropping the command output is safe — nothing reads it. The one gap is that the new non-empty allowed-team default extends the team check to mode: close teardowns, which the fork and label checks deliberately exempt.
Findings: 0 blocking | 4 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Changing
allowed-team's default from''tosei-protocol/sei-coreis a silent behaviour break for any existing caller that pins this reusable workflow and relies on the old default: on the next ref bump the comment path stops working, and the denial notice names a team the commenter has never heard of. It matchesai-review.yml's convention so it is clearly intended, butseidroid-review.ymlhas no README entry documenting its inputs (unlikeai-review.yml, whose table at.github/seidroid/ai-review/README.md:75already records this default), so nothing tells a caller what changed. Worth a release note on the tag that carries it. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
- 2 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
.github/workflows/seidroid-review.yml:639— the team membership read isgh api … 2>/dev/null || true, so a failed lookup is indistinguishable from a non-member: both fall through to the same$ACTOR is not an active member of $ALLOWED_TEAMnotice at line 640, and the API's own error (missingMembers: Read, org SSO, rate limit) is discarded. The neighbouring fork and label checks let stderr reach the log for exactly this reason. Now that the check runs on every comment-triggered dispatch rather than only when a caller opted in, the diagnostic loss is more costly. - [suggestion] The guard's own command grammar at
.github/workflows/seidroid-review.yml:517still accepts a bareseidroid reviewwhere the documented caller form requires@seidroid, so the guard and the caller can read one comment body differently. This PR works around the divergence by gating every check on$MODEinstead of reconciling the two parsers, soshould_runcan still betruefor a body the caller would never have dispatched. Aligning the guard's regex with the caller'strigger-phrasewould remove the second grammar rather than route around it.
| # silently stop reviewing every pull request opened by anyone outside the | ||
| # team, which is the opposite of what a caller sets this input for. | ||
| if [ "$EVENT_NAME" != "pull_request" ] && [ -n "$ALLOWED_TEAM" ]; then | ||
| if [ "$EVENT_NAME" != "pull_request" ]; then |
There was a problem hiding this comment.
[suggestion] This check runs for every non-pull_request event, which includes mode: close. Combined with the new non-empty default it now gates teardown for callers that never set allowed-team — a case that previously skipped the block entirely.
The PR description gives "A teardown is never affected — mode: close skips the check" as the third reason for the fail-closed posture. That holds for the label check (line 714) and the fork check (line 674), both of which guard on [ "$MODE" != "close" ], but not for this one.
The concrete case is a caller with no App configured. GH_TOKEN here is App-only with no GATE_TOKEN fallback, so line 638 denies @seidroid review close outright. That same caller can still start reviews — the review job authenticates with OMNIGENT_MACHINE_CLIENT_*, not the App — so it creates sandboxes it can no longer reclaim by comment, against the block below that notes "nothing else will: no lifetime cap, no sweep." Recovery exists only through the pull_request close event, which skips the guard.
Refusing a non-member's teardown is defensible as a security control, but the identity-missing denial is not a membership decision. Either exempt close the way the other two checks do, or scope the strictness so a missing App identity does not strand a session; and the input description at line 109 should say the gate covers teardown as well as review.
9c56ab5 to
f7add4a
Compare
There was a problem hiding this comment.
The three fixes are coherent and well-reasoned: gating both the fork and skip-label checks on inputs.mode removes the two-grammar divergence, the label read under GATE_TOKEN makes the label bite without an App, and allowed-team now fails closed. I verified nothing still reads the dropped command output or the APP_*_PRESENT env vars, and $ENV in --jq is already relied on elsewhere in this file. Remaining notes are a now-stale comment in the review job about the close path's gating, one misleading denial message, and a rollout dependency the PR's caller analysis doesn't cover.
Findings: 0 blocking | 4 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The PR body's verification matrix covers
close-labelled,close-label-read-failsandclose-body-review, but has no case for a comment-triggered close from an actor who is not an active member ofallowed-team— which is exactly the admission this PR newly widens. Worth aclose-nonmemberrow so the harness pins the new behaviour. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| # Reading an organisation's teams needs the App identity, so a caller that | ||
| # configures no App is refused here. The notice says so, and names the one | ||
| # thing that fixes it. | ||
| if [ "$EVENT_NAME" != "pull_request" ] && [ "$MODE" != "close" ]; then |
There was a problem hiding this comment.
[suggestion] Replacing [ -n "$ALLOWED_TEAM" ] with [ "$MODE" != "close" ] does two things at once: it makes an omitted team deny (PLT-1149, intended), and it exempts a comment-triggered teardown from the team gate entirely. For the two existing callers, which both pass allowed-team: 'sei-protocol/sei-core', that is a widening — a comment close was gated on active team membership before this change and is now gated only on the job condition's OWNER/MEMBER/COLLABORATOR association. The reasoning in the comment above (a sandbox nothing reclaims) is sound, but the change leaves the review job's own comment contradicting the code:
# A close asked for in a COMMENT is a different thing and does need one. It is a
# person destroying a session, so it goes through the same team gate the review
# does; the only ungated close is the one the platform itself reports.
That is no longer true — a comment close now skips the team gate. Please update that block so the two readings agree.
| # Admitting on a read that did not answer costs the label its meaning, on the | ||
| # one pull request whose author asked for no review. Anything but a plain | ||
| # `false` denies. | ||
| if [ "$MODE" != "close" ] && [ -n "$SKIP_LABEL" ]; then |
There was a problem hiding this comment.
[suggestion] Worth calling out for rollout: on the automatic pull_request path with no App configured, this check previously made zero API calls (the old if [ -n "$GH_TOKEN" ] short-circuited, and the fork check reads repository ids straight off the payload). It now always issues a repos/{owner}/{repo}/pulls/{n} read and denies when it fails — so a caller whose calling job relies on GitHub's default GITHUB_TOKEN permissions (contents/packages/metadata read, no pull-requests) loses every automatic review on a private repository, where the old code reviewed fine.
The PR body's "Both existing callers" analysis checks their allowed-team input and App secrets but not their permissions: blocks, which this check now hard-depends on. Confirming that both callers grant pull-requests: read on the calling job before bumping the pin would close the gap. The requirement is already stated in the job's permissions: comment, so this is a verification note rather than a code change.
| carries="$(GH_TOKEN="$GATE_TOKEN" \ | ||
| gh api "repos/$REPO/pulls/$PR" \ | ||
| --jq 'any(.labels[]?.name; . == $ENV.SKIP_LABEL)')" \ | ||
| || deny "could not read the labels on $REPO#$PR, so $SKIP_LABEL cannot be ruled out; not reviewing. Grant pull-requests: read on the calling job, or pass the App secrets, then ask again with @seidroid review" |
There was a problem hiding this comment.
[nit] The remedy is self-contradictory on the automatic path for the configuration that most often hits it. A caller with no App that also lacks pull-requests: read is told to "ask again with @seidroid review" — but that comment path is refused first by the team check at line 671, which requires the App. Either drop the trailing clause or name the App as part of the remedy, so the notice doesn't send the reader down a path that cannot work.
… and identity Three defects in the guard's `Admit the request` step, all in one step. PLT-1169: the skip-label check keyed on the guard's own parse of the comment body, where the caller's `mode` is what decides whether a review runs. The two grammars differ, so a labelled pull request whose body reads as a close was admitted with the label ignored, and a teardown whose body read as a review was refused. Both checks now gate on `$MODE`, and the guard stops deriving a command it no longer reads. PLT-1160: the label read used the App token alone, so a caller with no App got no label check. It now reads under GATE_TOKEN and fails closed. A refused read costs one review a person can retry; admitting on one costs the label its meaning. PLT-1149: `allowed-team` defaulted to empty, which skipped the only team gate on a comment-triggered review. It now defaults to `sei-protocol/sei-core`, and an empty value denies, matching ai-review.yml. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… a review is refused The team check gated `mode: close`. Its two neighbours, the fork check and the label check, exempt a teardown on purpose: a close is the only thing that reclaims a sandbox, and no lifetime cap or sweep reclaims one otherwise. A team check that refuses a close strands the pod. It now exempts a close the same way, so any collaborator the job condition admits may reclaim a sandbox. The refusal a caller with no App meets on the comment path now names its cause and its fix: the run holds no App identity, reading an organisation's teams needs one, pass both App secrets. It also names what still works without them, because a person reading a refusal wants a next step. The `allowed-team` input and the `SEIDROID_APP_ID` secret carry the same fact for the person configuring the caller. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…he label refusal a remedy that works The review job said a comment-triggered close goes through the same team gate a review does. It does not: the team, fork and label checks all exempt a teardown. The comment now names what does admit one — the comment grammar and the job's OWNER/MEMBER/COLLABORATOR filter. The label refusal told a caller to ask again with @seidroid review. A caller with no App is refused on that path by the team check, so the remedy named a route that cannot work. It now names the two that can: grant pull-requests: read on the calling job, or pass both App secrets. The guard's permissions comment records that pull-requests: read is load-bearing. The label check reads the pull request on every review, including an automatic one under GITHUB_TOKEN, and refuses when that read fails. GitHub's default token grants contents, packages and metadata only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
f7add4a to
740f013
Compare
|
All three fixed in Finding 1 — the
Finding 2 — the rollout dependency. The sharpest of the three, and I missed I re-checked both callers against the API rather than taking the numbers on
The PR body now carries the finding, the table, and the reason the next caller is One adjacent thing I found while checking, pre-existing and not from this PR: Finding 3 — the self-contradicting remedy. Agreed, and it is the same
The case you asked for. Verification. 46 cases, 13 verdicts changed, re-extracted from the pushed |
There was a problem hiding this comment.
The three fixes are sound: gating the skip-label and team checks on inputs.mode instead of the guard's own parse removes the two-grammar divergence, the label read under GATE_TOKEN makes the label bite for App-less callers, and exempting mode: close from the team check is consistent with the fork and label checks (a close only runs driver review --close, never checking out PR code). No blocking defects found; the notes below are about operability of the new fail-closed paths and one stale neighbouring comment.
Findings: 0 blocking | 5 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Verified that nothing reads the removed
commandoutput —steps.parse.outputs.commandhas no remaining references in the file, and no in-repo workflow callsseidroid-review.yml, so the removal is safe. Downstream callers (sei-load, sei-internal-skills) are the only consumers and should be checked for theallowed-teamdefault change before this is tagged. - 3 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] A guard refusal is only ever a
::notice::in the run log (deny()at .github/workflows/seidroid-review.yml:636), and the reaction step that acknowledges a comment lives in thereviewjob, which is skipped when the guard denies. A person who comments@seidroid reviewand is refused gets no signal at all on the pull request. This PR adds two more refusal paths on the comment path, which widens the surface, but the silent-refusal behaviour itself is on the base branch.
| carries="$(GH_TOKEN="$GATE_TOKEN" \ | ||
| gh api "repos/$REPO/pulls/$PR" \ | ||
| --jq 'any(.labels[]?.name; . == $ENV.SKIP_LABEL)')" \ | ||
| || deny "could not read the labels on $REPO#$PR, so $SKIP_LABEL cannot be ruled out; not reviewing. Grant pull-requests: read on the calling job, or pass SEIDROID_APP_ID and SEIDROID_APP_PRIVATE_KEY" |
There was a problem hiding this comment.
[suggestion] This denial is emitted as a ::notice:: (via deny()), which produces no visible marker on the run. Because the label check now runs on every review — including the automatic path for a caller with no App — a token that cannot read repos/{owner}/{repo}/pulls/{n} silently stops all reviews repo-wide, with the only evidence buried in the log of a run that shows green. That is exactly the failure the paragraph above anticipates ("a caller that takes the default reviews nothing on a private repository"). Consider emitting ::warning:: (or ::error::) for the unreadable case specifically, keeping ::notice:: for a genuine policy refusal — a fail-closed outage caused by misconfiguration deserves a different colour than "the author asked for no review".
| required: false | ||
| type: string | ||
| default: '' | ||
| default: 'sei-protocol/sei-core' |
There was a problem hiding this comment.
[suggestion] This is an intentional and documented breaking change for consumers, but it removes the only way to express "any collaborator the job condition admits may command a review": empty previously skipped the check and now denies, so a caller outside sei-protocol has no configuration that admits the comment path at all — they must either own a team in their own org or lose the feature. Combined with the App requirement for the membership read, upgrading to this ref silently turns every @seidroid review comment into a refusal for such a caller. Worth calling out in the release notes / a version bump for the tag downstream repos pin, since neither the guard nor the run status makes the change visible at upgrade time.
| # | ||
| # Every reader prefers the App identity and falls back to this, so a caller that | ||
| # configures no App still gets all three. A refused read costs them differently, | ||
| # configures no App still gets all four. A refused read costs them differently, |
There was a problem hiding this comment.
[nit] "all three" → "all four" here, but the sibling sentence ten lines above is now stale: "Three reads share it. The fork check reads the pull request. The once-per-PR gate reads a review... and a comment." The label read is a fourth read sharing pull-requests: read — and per the paragraph you added below it is the load-bearing one. Worth updating that enumeration in the same pass, since these comments are the only reference this workflow has.
An author can now ask for a polish pass. `seidroid-review.yml` never
passed
`--include-nits`, so `IncludeNits` was false on every review and the
driver's nit
setting 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-label` matches `ai-review.yml`'s input of the same name and its
`ai: nitpick` default (`ai-review.yml:66-70`), so a repository running
both tools
adds 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 fed
from 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 file
rather than `ai-review.yml`'s.
One semantic does not transfer. `ai-review.yml` re-runs on a `labeled`
event when
the 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`. The
input 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
under
`GH_TOKEN`, which is the App token with no fallback; a caller that
configured no
App would never be able to opt in. Answering that caller needs
`GATE_TOKEN`, which
is a separate `gh api` call whichever job it lives in — so sharing the
guard's call
would mean restructuring a fail-open/fail-closed admission check for a
signal that
decides nothing about admission. The guard also `deny`s by `exit 0`
mid-step, so an
output 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_TOKEN` there would hand the reviewing agent a GitHub credential.
The read uses `any(.labels[]?.name; . == $ENV.NITPICK_LABEL)`, the shape
#93 gives
the 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 --help` and refuses a driver missing any
long flag
this file passes, before a session opens or quota is spent.
`--include-nits` is now
on 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 already
admitted 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-driver` at `v0.15.0`,
which is
both the `driver-version` default and `MIN_DRIVER_VERSION` after #95:
| | label absent | label present |
|---|---|---|
| a nit-grade observation | `nitRule` sends it to `non_blockers`: prose
in the verdict comment and in the check run's Non-blocking section, no
thread on the code | reported inline with severity `nit`: a comment
thread on the line |
| a nit the review placed inline anyway | dropped — `PlaceableFindings`
(`findings.go:128`), the counts (`findings.go:373` via `countFindings`),
and the check summary, which renders only the line-less buckets | placed
and counted |
| a prior thread a nit restates | supersedes nothing, because no comment
posts | superseded, and resolved once the comment is on the code |
So 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 run
and 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 setting` and `Drive session + collect verdict` — were
extracted from
the shipped file with a YAML parser and run under `bash` with stubs. The
harness
asserts each step's `if` and the `INCLUDE_NITS` wiring against the file,
so a
rebase that changes one fails the harness rather than passing it. The
`gh` stub
runs the shipped `--jq` filter through real `jq`; the `go` stub serves a
driver
whose reported version and `review --help` flag set the case controls.
**1. driver argv**
```
case nit step --include-nits drive rc
label present include_nits='true' yes 0
label absent include_nits='false' no 0
no labels at all include_nits='false' no 0
no labels key include_nits='false' no 0
near-miss labels include_nits='false' no 0
caller renamed it include_nits='true' yes 0
label read fails include_nits='false' no 0
label input empty skipped no 0
close mode skipped no 0
every input set include_nits='true' yes 0
```
`every input set` exists so the union of flags below is the whole
surface. Its argv:
```
review sei-protocol/uci 42 --out .../verdict.md --findings-out .../findings.json \
--check-out .../check.json --conversation-context .../threads.json \
--guidelines-file REVIEW.md --extra-instructions "be terse" --include-nits \
--trigger-id 999
```
Close mode: `review sei-protocol/uci 42 --close`.
**2. install contract check**
```
case version mode rc annotation
help names every flag v0.15.0 review 0
help drops --include-nits v0.15.0 review 1 ...does not accept `review` --include-nits
help drops --check-out v0.15.0 review 1 ...does not accept `review` --check-out
driver below the floor v0.14.0 review 1 ...is older than v0.15.0
below the floor, close v0.14.0 close 0
```
The stub help gives half the flags a cobra shorthand (`-x, --out
string`), so the
check 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:
```
contract list: --out --findings-out --check-out --close --conversation-context
--guidelines-file --extra-instructions --include-nits --trigger-id
argv built: --check-out --close --conversation-context --extra-instructions
--findings-out --guidelines-file --include-nits --out --trigger-id
built but NOT in the contract list: none
in the contract list but never built here: none
```
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.0` was installed from the proxy and its
`review --help` names exactly `--check-out --close
--conversation-context
--extra-instructions --findings-out --guidelines-file --help
--include-nits --out
--trigger-id` — the contract list plus `--help`.
The `--jq` filter was also run through `gh`'s own engine
(`github.com/cli/go-gh/v2/pkg/jq`
v2.16.0): label present `true`, absent `false`, empty array `false`, no
`labels`
key `false`, `ai: nitpicky` `false`, `AI: Nitpick` `false`.
`actionlint` 1.7.12 on the same path with the same invocation: base
`30f5c09`
gives 4 findings, all `SC2102:info`; this branch gives 4, all
`SC2102:info`.
`shellcheck -S info` on all three extracted scripts: clean. The file
parses under
`yaml.safe_load`; 19 `workflow_call` inputs.
Not verified: any live run, and the flag's effect on a real model turn.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Three defects in the guard's
Admit the requeststep, shipped together becauseall three edit that one step. PLT-1169, PLT-1160 and PLT-1149.
$COMMAND, the guard's own parseof the comment body.
inputs.modedecides whether a review runs. The twogrammars differ, and both directions of the divergence are reachable. The
guard admitted a labelled pull request whose body reads as a close, and it
refused a teardown whose body reads as a review. Both checks now gate on
$MODE. The guard also drops acommandoutput that nothing reads, whichleaves one grammar in the guard.
ai: skip-reviewdid nothing for a caller with no App. It now reads under
GATE_TOKEN, and itfails closed.
allowed-teamdefaulted to empty, which skipped the only teamgate on a comment-triggered review. It now defaults to
sei-protocol/sei-core, and an empty value denies.Review round two adds one fix and one wording change:
mode: close. It did, on the base as wellas on the first revision of this PR. Its two neighbours exempt a teardown on
purpose, and it now does too.
fix, and what still works. The same fact now sits in the
allowed-teaminput description and the
SEIDROID_APP_IDsecret description.Review round three fixes three more:
reviewjob's comment matched the old gating. It said a comment closegoes through the same team gate a review does. The exemption made that false.
pull-requests: readis now load-bearing, and the PR body did not say so.See "The rollout dependency" below.
to retry with
@seidroid review, which the team check refuses first.Which checks gate a close
Every check in
Admit the request, tested rather than reasoned about. Theclose-*rows of the table below are the evidence.$PARSED)contains()filter is only a pre-filter.close-not-a-commandrefuses prose that quotes the command, and should.ifblockspull_requestwithmode: review, soEVENT_NAME=pull_requestimpliesMODE=review.close-auto-draftrefuses, and is unreachable while that condition holds.close-team-nonmemberpins the widening: the same actor and state thatteam-explicit-nonmemberrefuses for a review now reaches a close.$MODE != close)$MODE != close)ifblocksclose-auto-prior-verdictis unreachable while it holdsExactly one check gated a teardown and should not. Rows 2 and 6 are not defects
but they are a standing dependency: they refuse a close on the
pull_requestpath, and only the job
ifkeeps that pairing from arising. Anyone who widensthat condition has to revisit both.
This is a pre-existing defect, not one PLT-1149 introduced. On the base,
close-team-nonmember,close-team-malformedandclose-team-read-failsallrefuse the teardown. Both existing callers set
allowed-team, so both carrythe defect today. PLT-1149's default would have extended it to callers that omit
the input.
What the exemption widens. A close is now available to any collaborator the
job condition admits — OWNER, MEMBER or COLLABORATOR, non-bot — rather than to
the team alone. A close destroys a sandbox and nothing else. The alternative is
a pod holding reserved cpu and memory with no path to reclaiming it.
The rollout dependency:
pull-requests: readis now load-bearingOn the automatic path with no App, the label check used to make zero API
calls. The old
[ -n "${GH_TOKEN:-}" ]short-circuited, and the fork check readsrepository ids off the event payload. This PR makes that check always issue a
repos/{owner}/{repo}/pulls/{n}read, and refuse when it fails.A caller that takes GitHub's default
GITHUB_TOKENpermissions therefore losesevery automatic review on a private repository, where the old code reviewed
fine. That default grants
contents,packagesandmetadataread, and nopull-requests. My earlier caller analysis checkedallowed-teamand the Appsecrets. It did not check
permissions:, which this check now hard-depends on.I re-checked both callers directly against the GitHub API rather than from
memory:
permissions:pull-requests: read?sei-loadcontents: read, pull-requests: write, checks: write, issues: writewritesubsumesreadsei-internal-skillscontents: read, pull-requests: write, checks: writeNeither breaks. Only the guard's
permissions:comment implied therequirement before. This PR states it there in as many words: the grant is
load-bearing rather than declared, and a caller on the default token reviews
nothing on a private repository.
One adjacent observation, pre-existing and not from this PR:
sei-internal-skillsgrants noissues:scope at any call site, while the guardjob declares
issues: read. Per the docs a called workflow may only downgrade,and the docs do not say what happens when it asks for more. Either GitHub errors
that job on the next pin bump, or it downgrades to
noneand the gate's commentread fails open with a warning. Worth resolving before that caller bumps.
Posture decision for the label check: fails closed
A read that does not answer refuses the review. Three facts weighted, in order:
check that landed in fix(seidroid-review): refuse an explicit re-review on a fork-originated pull request #89 reads
repos/{owner}/{repo}/pulls/{n}underGATE_TOKENand refuses when it cannot place the pull request. A label checkthat admits on that same failed read would give two answers to one API error.
review, and the notice names both fixes. Admitting costs the label its whole
meaning, on the one pull request whose author asked for no review.
mode: closeskips the check, so a failedlabel read can never strand a sandbox — the failure that has no other
recovery.
The once-per-PR gate below still fails open, and its comment now says so against
this one rather than agreeing with it.
What GitHub's documentation actually says
Read from docs.github.com, API version 2022-11-28:
GET /repos/{owner}/{repo}/pulls/{pull_number}pull-requests: readGET /repos/{owner}/{repo}/issues/{issue_number}/labelsGET /orgs/{org}/teams/{slug}/memberships/{user}permissions:key has nomembersscope, so aGITHUB_TOKENcannot carry itThe third row is why the team check keeps
GH_TOKENand gains no fallback: theApp identity is the only identity that can answer it. The
GATE_TOKENcommentnow records that as the documented reason rather than an assertion.
One rule I could not fully confirm: the reusable-workflow reference states that
"the
GITHUB_TOKENpermissions passed from the caller workflow can be onlydowngraded (not elevated) by the called workflow." It does not say what happens
when a called workflow requests more than the caller granted. Nothing here ran
on a GitHub runner, so I did not test it.
Verification
Nothing in this PR ran on a GitHub runner. A harness reads the
parseandAdmit the requeststeps out of the YAML with PyYAML —jobs.guard.steps[]—and runs each under
bash. Aghstub onPATHserves fixture JSON throughthe step's own
--jqfilter and the realjq. The harness resolves every${{ }}in both steps'env:blocks from the workflow file. It hard-errorson an expression it does not know, so it cannot quietly stop modelling the
step it tests. Input defaults come from the file's own
workflow_call.inputs,so a case that omits an input models a caller that omits it.
The base moved three times:
5f5fd78to5d06528mid-task, then41ee3ff(#90), then
30f5c09(#95). Every case below comes from a fresh extraction ofthe rebased file.
#95moveddriver-versiontov0.15.0. Its hunks land at lines 99, 106 and1035+; the first hunk in this diff is at 109. The
guardjob is byte-identicalbetween
41ee3ffand30f5c09, dumped and diffed the same way as before.The rebase onto
41ee3ffreported no conflict, so I checked it rather thantrusted it. The
guardjob is byte-identical between5d06528and41ee3ff— dumped and diffed. #90's hunks land at lines 1151+ and 2292+, clearof every hunk in this diff. #90 also hoisted
FINDING_MARKERinto the workflowenv:block. The harness now exports every workflow env key rather than theone it used to name, so a later hoist reaches these scripts the way it does on a
runner.
admitas the step wrote it, base5d06528against this branch:Thirteen verdicts change. Each one is a ticket or a review finding asking for it:
divergence-labelledclose-body-reviewauto-noapp-labelledauto-noapp-read-failsauto-halfapp-baregithub.token, so the half-credential refusal loses its premiseteam-omitted-nonmemberteam-omitted-unknownteam-explicit-emptycomment-noapp-labelledcomment-noapp-bareclose-team-nonmemberclose-team-malformedclose-team-read-failsA no-App caller on the comment path: accepted, and the refusal says why
The automatic
pull_requestpath is unaffected. A reader will assume thathalf broke, so it goes first.
auto-noapp-bareadmits.auto-noapp-labelledrefuses on the label. No team check runs on that path at all. A no-App caller
keeps automatic reviews, and after this round keeps
@seidroid review close.The comment path is what changes.
allowed-teamis non-empty by default. Theteam check needs "Members" organization read, and only the App token carries it.
A caller with no App therefore meets a refusal when it asks for a review by
comment.
I accept that, for three reasons. It fails closed, and a gate that decides who
may spend a sandbox must refuse a claim it cannot verify. It matches
ai-review.yml, which defaults the same input and denies on empty. And it isnot new coupling: any caller that sets
allowed-teamhas it today.The refusal now reads:
The label check's own refusal follows the same standard. It used to end
"then ask again with @seidroid review", which sends a no-App caller to the one
path the team check refuses first. It now reads:
Cause, fix, and what still works. The
allowed-teaminput description and theSEIDROID_APP_IDsecret description carry the same fact, because the personconfiguring the caller and the person reading a refusal are different people.
One correction to the instruction. The review asked the notice to name two
ways out: configure
SEIDROID_APP_ID, or setallowed-team: ''. The secondone does not work. PLT-1149 makes an empty
allowed-teamdeny, and the samereview round accepted that change. A person who follows that advice meets
allowed-team is empty or is not org/team-slug; denying. Only one way outexists, and the notice names it. The input description says so in as many words:
"Setting this input empty is not the way out: empty denies."
Reverting empty-denies would restore the second way out and re-open half of
PLT-1149. That is the ticket owner's call, not one for me to make silently.
Both existing callers
sei-protocol/sei-loadandsei-protocol/sei-internal-skillsboth passallowed-team: 'sei-protocol/sei-core'on their review and close jobs, and bothconfigure the App.
team-explicit-member,team-explicit-nonmember,comment-app-*andauto-app-*are unchanged, so their behaviour holds whenthey bump their pin.
Their third job,
seidroid-review-reclaim, omitsallowed-team. It fires apull_requestevent withmode: close. The guard'sifdoes not match thatpair, so GitHub skips the guard and nothing reads the input. Both callers carry
a comment calling the input "optional there (default '')". That parenthesis goes
stale with this PR, though the behaviour does not change. Worth a follow-up edit
in those repositories.
actionlint
Rule set unchanged. Four
SC2102before and after, at the same offsets insidethe extracted scripts.
Grouping the parse step's three
>> "$GITHUB_OUTPUT"writes is what keeps thatset unchanged. Removing the
command=classification left three adjacentredirects, which raised a new
SC2129; the{ … } >> fileform matches theshape the same step's
pull_requestbranch already uses.One near-miss worth recording, because it is the same class of defect the
review warned about. My first "final" verification read
origin/fix/guard-admission-parity, whose local tracking ref had not moved pastthe force-push. It served the pre-fix tree and produced a table that disagreed
with the working tree.
git ls-remotesaidf7add4a; the tracking ref said9c56ab5. The numbers above come from a re-fetched ref, and the file behindthem is byte-identical to the working tree (
cmp).Also checked, on the changed file:
shellcheck -s bashon the extractedAdmit the requestscript: clean.$VARin every guardrun:script resolves: a step, job or workflowenv:key declares it, the script assigns it, or the script reads it as${VAR:-}. Everyenv:key has a reader. That is theset -ucheck.Dropping
COMMAND,APP_ID_PRESENTandAPP_KEY_PRESENTmust not strandone.
What I did not verify
shell against a stub.
permission its caller did not grant.
failure and a non-zero
gh apiexit; it does not model a partial page or arate limit.
--paginatedoes notapply, but the stub serves one page for the gate's reads as well.
Accepted, not fixed
On the comment path the guard now reads
repos/{owner}/{repo}/pulls/{n}twice:once for the fork check, once for the label. Folding them into one read means
restructuring the block #89 just landed. The automatic path saves nothing
either, because it reads the fork signal from the event payload. One extra REST
call, against a review that holds a sandbox for minutes, does not pay for that
coupling.
Follow-ups, not done here
seidroid-review-reclaimjob carries a comment callingallowed-team"optional there (default '')". The parenthesis goes stale withthis PR. The behaviour does not change, because that job never reaches the
guard. Recorded here as a follow-up in
sei-loadandsei-internal-skills; Idid not edit either repository.
ai-review.ymldoes. The input descriptions in the file are the only reference, and three of
them changed here.
Corrections to the three tickets
check". It denies inside the check, as its
elifbranch. That branch restson one premise: half a credential mints no token, so nothing can read the
label. The fallback to
github.tokenends that premise, so this PR drops thebranch. The
Report a half-configured reviewer identitystep still names themissing half.
comment path refuses". The existing behaviour admits: an empty
allowed-teamskipped the check. This PR implements the refusal the sentenceasks for. That matches ai-review.yml and the ticket's own thesis. The
"existing behaviour" clause is wrong about the present.
Closes PLT-1169. Closes PLT-1160. Closes PLT-1149.
🤖 Generated with Claude Code