fix(seidroid-review): refuse an explicit re-review on a fork-originated pull request - #89
Conversation
PR SummaryHigh Risk Overview Operational fixes: Reviewed by Cursor Bugbot for commit b8318b6. 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 fork-origin gate is well-reasoned and its fail-closed jq/shell logic is sound, but it keys on the guard's own re-parse of the comment body ($COMMAND) rather than on inputs.mode, which is what actually decides whether a review runs — a divergence between the caller's routing and this parse either bypasses the gate on fork code or blocks the sandbox-reclaiming close.
Findings: 1 blocking | 4 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- The
SEIDROID_APP_IDsecret description (line 264) still says the only cost of omitting it is attribution. After this change, omitting it also denies every comment-triggered review. The PR body flags this as a deliberate trade-off, but the declared contract should say so. - 2 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] .github/workflows/seidroid-review.yml guard step: the guard re-parses the comment body into
commandwhile the review job's behaviour is driven entirely by the caller-suppliedinputs.mode. Two independent implementations of the same grammar live in different repositories with no consistency check, which is what makes both the new fork gate and the existing skip-label gate (line 512) divergence-sensitive.
| # | ||
| # It stops a REVIEW, not a teardown, for the reason the label check states. A | ||
| # fork pull request must still be able to reclaim its sandbox. | ||
| if [ "$EVENT_NAME" != "pull_request" ] && [ "$COMMAND" != "close" ]; then |
There was a problem hiding this comment.
[blocker] This gate keys on $COMMAND — the guard's own re-parse of the comment body — but what actually runs is decided by inputs.mode, which the caller derives from the same body independently (MODE: ${{ inputs.mode }} at line 816; every review step gates on inputs.mode == 'review', and the review job's issue_comment branch at line 562 never consults $COMMAND). Any divergence between the caller's routing and this parse breaks the check in both directions:
- Caller passes
mode: reviewwhile the guard parsesclose→ the fork check is skipped and a full review runs on fork code. This is reachable in practice: the guard's grammar accepts a bareseidroid review closewith no@(line 349), so a caller whose routing keys on the documented@seidroid review closeform routes that comment as a review while the guard reads it as a close. - Caller passes
mode: closewhile the guard parsesreview→ the check runs and denies, blocking the only thing that reclaims a fork PR's sandbox — the case the comment above explicitly sets out to preserve.
Gating on the authoritative signal removes the dependence entirely, and is safe in both directions since nothing risky runs when mode is close. Add MODE: ${{ inputs.mode }} to the step env and use:
if [ "$EVENT_NAME" != "pull_request" ] && [ "$MODE" != "close" ]; thenThere was a problem hiding this comment.
Fixed in cb48dff. The gate now reads MODE, and MODE: ${{ inputs.mode }} is in the step env.
You were right that it is reachable, and I confirmed the bypass rather than taking it on trust. I extracted the Admit the request script from both revisions and ran the divergence case against each, with a gh stub reporting a fork:
COMMAND=close MODE=review GH_ORIGIN_MODE=fork
previous revision (keyed on $COMMAND) -> admit=true
this revision (keyed on $MODE) -> admit=false
admit=true on a fork is exactly the outcome this PR exists to prevent, so this was a real hole in the fix itself.
Three cases now cover the divergence, all passing:
| case | verdict |
|---|---|
fork + body parses close, caller sent mode: review |
DENY — fork refusal |
fork + body parses review, caller sent mode: close |
ADMIT |
same-repo + body close, caller mode: review |
ADMIT |
The comment above the check states why it reads the caller's mode and not this guard's own parse.
One thing your finding turns up that I have not fixed. The skip-label check at line 535 keys on $COMMAND too, so the same divergence reaches it. A bare seidroid review close, routed as mode: review, skips the label check. A labelled pull request then gets a review. Lower consequence than the fork hole: an unwanted review, rather than outside code next to a credential. It is also not this ticket. I left it alone to keep this diff rebaseable against #86, which edits the same step. Worth its own ticket.
Not verified: whether any caller's routing actually diverges today. I do not have the caller repos and the task scope excludes editing them. The reachability argument rests on the two grammars, which are both in this repository — @?seidroid at line 371 here, against the documented @seidroid form.
| # It stops a REVIEW, not a teardown, for the reason the label check states. A | ||
| # fork pull request must still be able to reclaim its sandbox. | ||
| if [ "$EVENT_NAME" != "pull_request" ] && [ "$COMMAND" != "close" ]; then | ||
| [ -n "${GH_TOKEN:-}" ] || deny "no identity to read where $REPO#$PR comes from; denying" |
There was a problem hiding this comment.
[suggestion] This turns SEIDROID_APP_ID into a hard requirement for the comment path: with no App identity the guard holds no token, so every comment-triggered review now denies. Previously an identity-less caller was fine — the team check is skipped when allowed-team is empty and the label check fails open.
The PR body weighs this trade-off, but the secret's own description (line 264, "Optional: without it the review posts as the workflow's own identity...") still names attribution as the only cost, so a caller has no way to learn the new behaviour except by watching reviews silently stop. Please update that description. A ::warning:: rather than a ::notice:: for this specific deny would also make the misconfiguration visible in the run summary, since it is a wiring error rather than a normal refusal.
There was a problem hiding this comment.
This finding is stale as of the rebase, and I have not changed the description. Here is the check rather than the assertion.
The read no longer uses GH_TOKEN. It uses GATE_TOKEN: ${{ steps.identity.outputs.token || github.token }}, so the guard always holds a token, and the guard job now carries pull-requests: read for the fallback to work. I dropped the precondition you are commenting on, [ -n "${GH_TOKEN:-}" ] || deny, because GATE_TOKEN made it unreachable.
The no-App rows in the stub run, with the identity step skipped and GATE_TOKEN holding github.token:
| case | verdict |
|---|---|
| no App, same-repo PR + review comment | ADMIT |
| no App, fork PR + review comment | DENY — fork refusal |
no App, fork PR + close |
ADMIT |
A caller with no App still gets its reviews. The description at line 264 stays correct, so editing it would make it wrong.
On ::warning:: rather than ::notice::. Declining, and saying why rather than just skipping it. The deny you attached this to no longer exists. The one unreadable-signal deny that remains prints gh's own error immediately above the notice, which I verified:
gh: Resource not accessible by integration (HTTP 403)
::notice::could not read where sei-protocol/uci#42 comes from, so a fork cannot be ruled out; not reviewing it
#86 does use ::warning:: for an unreadable signal, but for the opposite outcome: it proceeds, so the warning is the only record left. This one denies, and the deny notice is itself the record. Raising the level would mean bypassing the shared deny() helper for one call site, or emitting two annotations for one event. Happy to take it if you would rather have the run-summary prominence — say so and I will.
Not verified: that github.token with pull-requests: read answers this endpoint in a real runner. The stub models it. I have not executed a run.
| # fork pull request must still be able to reclaim its sandbox. | ||
| if [ "$EVENT_NAME" != "pull_request" ] && [ "$COMMAND" != "close" ]; then | ||
| [ -n "${GH_TOKEN:-}" ] || deny "no identity to read where $REPO#$PR comes from; denying" | ||
| origin="$(gh api "repos/$REPO/pulls/$PR" \ |
There was a problem hiding this comment.
[nit] This hits repos/$REPO/pulls/$PR and the skip-label check at line 513 hits the exact same endpoint a few lines later. One fetch reused for both would drop a round trip and keep the two checks reading a single consistent snapshot of the PR.
There was a problem hiding this comment.
Leaving them separate. The saving is real but it costs a boundary I would rather keep.
The two checks have opposite failure postures on purpose: this one fails closed, the label check fails open. Sharing one fetch entangles them in three ways.
The tokens differ, and that is not incidental. This read uses GATE_TOKEN. The label check uses GH_TOKEN, and skips itself entirely when a caller configures no App. A shared GATE_TOKEN fetch would start enforcing the skip label for callers where it is inert today. That is arguably an improvement. It is also a user-visible behaviour change outside a security fix. A no-App caller whose pull request carries ai: skip-review would suddenly lose its reviews. Nothing would tell them why.
The run conditions differ. This check runs on the comment path with mode != close. The label check runs on both paths with command != close. A shared fetch has to hoist above both, under the union of those conditions. That puts a fetch on the automatic path under a condition neither check states on its own.
The postures stop being separately observable. If a shared read fails, this check denies and exits before the label check runs. The label check's fail-open would no longer show on that path. Today that holds only on the comment-review path. Sharing would make it structural.
Keeping them apart costs one extra API call, on one path, in a job with a five-minute timeout. A person is already waiting on that request. I would not trade one round trip for an entangled fail-closed and fail-open pair.
One practical reason sits behind that. #86 adds two more reads to this same step and lands first. Restructuring the step around a hoisted shared fetch would make that rebase materially harder for no behavioural gain.
Not verified: the actual latency of the extra call. I am reasoning from it being one REST GET on a path a person is already waiting on, not from a measurement.
1d70be7 to
717f4bb
Compare
There was a problem hiding this comment.
Correct, well-scoped security fix: the fork check fails closed, admits only a definite same, correctly exempts the close teardown path (which does no checkout and runs no agent), and effectively blocks the review job via admit=false. No blocking issues; two non-blocking notes about the neighbouring skip-label check and the silence of the new deny path.
Findings: 0 blocking | 2 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- A denied fork re-review is silent to the requester: the deny writes only a
::notice::to the run log, and the eyes-reaction step lives in the skippedreviewjob, so a collaborator who comments@seidroid reviewon a fork PR sees nothing happen at all. This matches how the file's other denies behave (draft, team, skip-label), so it isn't a regression — but this is the one deny a collaborator with full repo rights will plausibly hit and be confused by. Consider a reaction or a one-line comment on the refusal. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| # team check has no such fallback. Reading an organisation's teams needs | ||
| # an identity that can see them, and GITHUB_TOKEN cannot. Reading a pull | ||
| # request needs no more than pull-requests: read. | ||
| GATE_TOKEN: ${{ steps.identity.outputs.token || github.token }} |
There was a problem hiding this comment.
[suggestion] Now that GATE_TOKEN exists and the guard job carries pull-requests: read, the skip-label check below (line 535) could use it too. It currently gates on [ -n "${GH_TOKEN:-}" ], so for a caller that configures no App the skip label is silently inert — the label check fails open on "no identity" even though github.token can now read .labels[].name on the same endpoint this new check already calls. Its fail-open semantics would be preserved (2>/dev/null + no match ⇒ admit); only the "no identity to ask with" case changes, which is exactly the case the new token was added to cover.
Minor, and arguably out of scope for a security fix — but worth noting since the two checks now hit the identical repos/$REPO/pulls/$PR endpoint with different tokens.
There was a problem hiding this comment.
Agreed on the observation. I am declining the change here. It deserves its own ticket, rather than a quiet fix inside a security PR.
You are right on the mechanism. github.token with pull-requests: read can now read .labels[].name on the endpoint this check already calls. The [ -n "${GH_TOKEN:-}" ] gate is the only thing left keeping the skip label inert for a no-App caller.
Why not here. That change is not posture-preserving in the way it looks. Today a no-App caller's ai: skip-review label does nothing, and their reviews run. Switching the token makes the label bite, so a pull request carrying it loses its review. The caller cannot tell that apart from the reviewer breaking, and nothing on the pull request says otherwise. It is the right end state. It is also a behaviour change, and it should arrive under its own heading where a caller can read about it. Not inside a fork refusal.
It also touches the same step #86 edits, and #86 lands first. I am keeping this diff to the smallest thing that closes the hole.
Not verified: whether any live caller actually sets skip-review-label without an App. That is what decides whether the change is invisible or disruptive, and I cannot see the caller repos from here.
Superseded: latest AI review found no blocking issues.
717f4bb to
cb48dff
Compare
There was a problem hiding this comment.
The fork guard is soundly built: it fails closed on an unreadable origin, compares repository ids rather than names, treats a null head repo as a fork, keys on the caller's mode so a teardown on a fork can still reclaim its sandbox, and the new pull-requests: read grant is no new burden on callers since the review job already requires pull-requests: write. One defence-in-depth gap remains: the pull_request path is exempted on the assumption that GitHub always withholds secrets from fork runs, which a private/internal repository's Actions settings can override.
Findings: 0 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The file's header block (lines 14-18) enumerates the gates — "the automatic path refuses a draft and honours the skip-review label, and the manual path additionally checks who is asking" — but is not updated for the new fork refusal. Given how thoroughly this file documents itself, adding "and refuses a fork-originated pull request" to that inventory keeps the one summary a reader starts from accurate.
- The fork check and the skip-label check below it each issue their own
gh api "repos/$REPO/pulls/$PR"call. Fetching the PR once into a variable and deriving both signals from it would halve the API calls in the guard, though the two checks' differing fail-open/fail-closed postures make the merge non-trivial, so this is optional. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| # accepts a bare `seidroid review close`. A caller matching the documented | ||
| # `@seidroid` form reads that same comment as a review. Mode decides what the | ||
| # review job does, so mode is what this gates on. | ||
| if [ "$EVENT_NAME" != "pull_request" ] && [ "$MODE" != "close" ]; then |
There was a problem hiding this comment.
[suggestion] The $EVENT_NAME != "pull_request" exemption rests on the comment's claim that "GitHub withholds this workflow's secrets from a fork pull_request run". That is the default and is unconditionally true for public repositories, but private and internal repositories have an Actions setting ("Send secrets and variables to workflows from fork pull requests", settable per-repo or by org policy) that disables it. Where that is enabled, an automatic fork pull_request run would receive OMNIGENT_MACHINE_CLIENT_SECRET, pass the Require the machine-client secret step, and drive an agent over fork code — exactly the exposure this check exists to close, on the one path it skips. Note the pull_request branch of the guard also carries no author-association check, so nothing else on that path stands in for it.
Dropping the $EVENT_NAME condition and leaving only [ "$MODE" != "close" ] costs nothing observable: an automatic fork review cannot run today under default settings, so no working behaviour changes, and the fail-closed posture then holds on both paths rather than on one path plus an account setting. For the pull_request event the ids are already in the payload (github.event.pull_request.head.repo.id / .base.repo.id), so that path need not spend the API call at all.
There was a problem hiding this comment.
Taken, both halves. d7622be.
I dropped the $EVENT_NAME condition. The gate is now [ "$MODE" != "close" ] alone, so it covers both paths. The posture no longer rests on an Actions setting that no caller of this file controls.
Your second half too: the pull_request payload already carries both ids, so that path spends no API call. I measured it rather than assuming it, by counting stub invocations:
| path | origin API reads |
|---|---|
pull_request, same-repo payload |
0 |
pull_request, fork payload |
0 |
pull_request, mode close |
0 |
issue_comment, review |
1 |
issue_comment, mode close |
0 |
On keeping the payload path fail-closed. One trap here does not exist on the API path. Comparing two ids that are both empty makes them equal. That would admit a fork on a payload carrying no signal at all. The payload branch therefore tests the base id first and treats an absent one as unreadable, not as a match:
if [ -z "$BASE_REPO_ID" ]; then
origin=unreadable
elif [ "$HEAD_REPO_ID" = "$BASE_REPO_ID" ]; then
origin=same
else
origin=fork
fiA present base id with an absent head id — the deleted-fork case — falls to fork, which is the reading you asked for.
Both paths proved:
| case | verdict |
|---|---|
pull_request + fork payload |
DENY — #42 is fork-originated; not reviewing it |
pull_request + same-repo payload |
ADMIT |
pull_request + null head repo id |
DENY — fork |
pull_request + no ids at all |
DENY — could not read |
pull_request + fork + draft |
DENY — draft, unchanged |
pull_request + mode close |
ADMIT |
The refusal wording differs by path on purpose. explicit re-reviews are disabled for fork-originated pull requests is the incumbent's sentence and it is accurate on the comment path. On the automatic path nobody asked explicitly, so that path says $REPO#$PR is fork-originated; not reviewing it, matching the shape of the draft and label denies beside it.
Two comments your finding falsified, which I also fixed. The claim you are quoting appeared twice more. Leaving either would have left a false rationale in the file:
- The check's own comment said the automatic path never reaches it. It now names three facts. GitHub withholds the secrets by default. A private or internal repository can turn that off. This check does not rest on that setting.
- The guard job's comment at line 318 justified having no author-association check on the
pull_requestbranch. It rested partly on the same withholding. It now points at this gate instead.
Not verified: I have no repository with that Actions setting enabled, so I have not observed a fork pull_request run receiving secrets. I take the setting's existence and effect from your description. Not depending on the default is sound either way.
cb48dff to
d7622be
Compare
|
Header inventory updated in The block was one long sentence carrying three clauses, already 35 words before this change. I split it rather than adding a fourth clause to it:
On sharing the PR fetch with the skip-label check. Declining again, for the three reasons in this thread. The two checks read with different tokens. They run under different conditions. A shared read makes the label check's fail-open structurally unreachable. The last one is the one I care about. Halving the guard's API calls is not worth making a deliberate posture difference invisible. The count is now one call on one path anyway, since the automatic path reads the ids from the payload. On the denied requester seeing nothing. Noted, not taken here, and worth its own ticket. You are right that this is the deny a collaborator with full repository rights will plausibly hit. Two things stop it being a one-line fix. The guard job holds Pre-existing issue you raised, on the two grammars. Agreed. PLT-1169 now tracks it. The skip-label check at line 535 still keys on Not verified: whether any caller's routing diverges from this guard's grammar today. I cannot see the caller repositories from here. |
There was a problem hiding this comment.
The fork check is sound: it compares repository ids rather than names, treats an absent/null head id as a fork, fails closed on an unreadable origin on both the payload path and the API path, and exempts close so a fork PR can still reclaim its sandbox. Two non-blocking points: the new deny on the automatic path is still followed by a hard failure of the machine-client step, and the PR description describes only the comment path while the commit changes both.
Findings: 0 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Behaviour divergence worth confirming:
ai-review.ymlblocks only explicit re-reviews on forks (!isAutomatic && isFork) and still reviews fork PRs automatically. This change refuses both paths, so external-contributor PRs lose automated review coverage once callers migrate. The commit message argues the case (a private/internal repo can be configured to pass secrets to fork runs, and no caller controls that setting), which is reasonable — flagging it as a deliberate posture change rather than a port of the incumbent's rule. - The PR description is stale against the commit: it says "Three edits ... +46/-1" and shows only the comment-path
if [ "$EVENT_NAME" != "pull_request" ] && [ "$MODE" != "close" ]form, while the merged code is +82/-6 and gates both paths, with a separate payload-based branch forpull_request. For a security-relevant guard change, the description is what a later auditor reads first — worth updating it to match. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| else | ||
| origin=fork | ||
| fi | ||
| refusal="$REPO#$PR is fork-originated; not reviewing it" |
There was a problem hiding this comment.
[suggestion] This deny is not the last word on the automatic path. Require the machine-client secret (line 582) is gated on steps.parse.outputs.should_run == 'true', not on admit, so it still runs after deny sets admit=false and exits 0. A fork pull_request run on a public repository receives no secrets, so SECRET_PRESENT is false, the step emits ::error::OMNIGENT_MACHINE_CLIENT_SECRET is not set on the calling repository and exits 1 — the Guard job goes red on every external contribution, pointing at a caller misconfiguration that isn't there. That failure was the intended mechanism before this change; now that the fork check owns the refusal, the leftover failure contradicts the notice it just wrote. Adding steps.admit.outputs.admit == 'true' to that step's condition keeps the fail-fast for admitted runs and lets a refused fork end as the clean notice this block is written to produce. (The comment path is unaffected — issue_comment runs do receive secrets.)
There was a problem hiding this comment.
Taken. a7bc29a. Require the machine-client secret now reads the verdict as well as the parse:
if: steps.parse.outputs.should_run == 'true' && steps.admit.outputs.admit == 'true'I checked the ids against the file rather than taking them from the report: the step id is admit and its output is admit, so steps.admit.outputs.admit resolves.
Proved by evaluating the condition the way GitHub would for this expression shape, then running the step's own script when it holds. A fork pull_request run on a public repository, refused by the gate, with no secrets:
| revision | condition | step | guard |
|---|---|---|---|
d4780ce |
should_run == 'true' |
runs, exit 1 | RED, ::error::OMNIGENT_MACHINE_CLIENT_SECRET is not set… |
a7bc29a |
should_run == 'true' && admit == 'true' |
skipped | GREEN |
The fail-fast survives where it belongs:
| scenario | machine-client step | guard | guard.should_run |
|---|---|---|---|
fork pull_request, no secrets, refused |
skipped | GREEN | false |
| admitted review, secret missing | runs, exit 1 | RED | true |
| admitted review, secret present | runs, exit 0 | GREEN | true |
| comment parsed to nothing | skipped | GREEN | false |
The review job skips either way, since guard.should_run is already parse && admit. Only the guard's colour changes.
The other step that keys on the parse alone, and why I left it. Report a half-configured reviewer identity also reads steps.parse.outputs.should_run == 'true'. Gating it on admit would be wrong. #88 made Admit the request deny when a caller sets half an App credential, and that step's warning is what explains the deny. Reading admit there would suppress the diagnostic exactly where a reader needs it. Those are the only two steps in the guard that key on the parse, so nothing else is affected.
One correction to the reasoning, which does not change the fix. The pull_request fork path does not fail today in every configuration. Your earlier finding named the Actions setting: "Send secrets and variables to workflows from fork pull requests", on a private or internal repository. Such a run does receive the secret, passes this step, and reviews fork code. This gate therefore removes a working behaviour there. That removal is the objective rather than a cost. The PR body now records it under its own heading, so nobody later reads it as an accidental over-port.
Not verified: none of this has run in a GitHub runner. My evaluator handles the && of a.b.c == 'literal' terms only, which is the shape of both conditions here. It is not GitHub's expression engine.
d7622be to
a7bc29a
Compare
There was a problem hiding this comment.
Adds a fail-closed fork-origin gate to both the automatic and comment paths of the reusable seidroid review workflow, using the pull_request payload ids where available and a gh api read otherwise. The logic is sound — all admit branches set an output, unset/empty ids are treated as unreadable rather than equal, and the new pull-requests: read on the guard is already implied by the review job's pull-requests: write — so no blocking issues; one design trade-off is worth calling out.
Findings: 0 blocking | 3 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The comment-path fork check now makes every comment-triggered review depend on a successful
repos/{repo}/pulls/{n}read that returns nothing but an id comparison. That is a deliberate fail-closed choice and is documented, but it means a single API hiccup or an under-privileged App installation refuses the review outright, whereas the sibling label read one block below degrades gracefully. Worth confirming on a private repo thatpull-requests: readalone (withcontents: none) is sufficient for that endpoint before relying on it as the only path when no App is configured — the fallback token is the one that will actually be used there. - 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]
.github/workflows/seidroid-review.yml:592— the skip-label check is keyed on$COMMAND(this guard's own re-parse of the comment body) while the new fork check is keyed on$MODE(what the caller routed). The new code block states the reason mode is the correct key: "Mode decides what the review job does, so mode is what this gates on." By that same reasoning the label check is keyed on the wrong signal — a dispatch the caller routed asmode: reviewbut that this guard parsed ascommand=closeskips the skip-review label and runs a full review on a labelled pull request. Pre-existing, but the two checks now disagree in the same step.
| # accepts a bare `seidroid review close`. A caller matching the documented | ||
| # `@seidroid` form reads that same comment as a review. Mode decides what the | ||
| # review job does, so mode is what this gates on. | ||
| if [ "$MODE" != "close" ]; then |
There was a problem hiding this comment.
[suggestion] This gate is unconditional and has no opt-out input, which changes behaviour for callers whose forks are not "outside the organisation". Internal and private-org repositories routinely use member-owned forks as the normal contribution flow; on those, every automatic review silently stops with no way to restore it short of editing this file. The other two gates in this step are both configurable (allowed-team, skip-review-label).
Consider a review-forks (or similarly named) workflow input defaulting to false, so the secure posture stays the default while an org that forks internally can opt back in — rather than making fork-vs-same-repo a proxy for trust that a caller cannot override.
…ted postures (#91) Three input declarations, one pull request. All three edit adjacent lines of the `workflow_call` inputs block, so three separate pull requests would serialise three rebases on one block. Brandon Chatham made each of the three decisions; this pull request records them. - **PLT-1163 — the default scout set.** The only behaviour change here. `scouts` defaults to `codex=xreview-scout-codex`, so a review reads on two models. - **PLT-1157 — the accepted `allow-tools` posture.** Prose only. The default stays `Bash,Read`. - **PLT-1151 — the accepted deployment.** Prose only. The default stays the development seigent URL. ## The default is `codex=xreview-scout-codex`, not the bare bundle name PLT-1163 asks for `xreview-scout-codex`. That value refuses every review. The driver parses the list as `name=agent` and treats a missing `=` as a configuration error: ``` {name: "no separator", raw: "codex", wantErr: true}, ``` `sei-agent-driver/cmd/sei-agent-driver/main_test.go` at tag `sei-agent-driver/v0.14.0`, which is this file's pinned `driver-version`. `parseScouts` in `main.go` returns `ErrConfig` for such an entry, and `main` turns that into `ExitConfig` before any turn starts. The default therefore carries the name, and `codex` is the name the driver's own README uses for this bundle. `agents/xreview-scout-codex` is the only scout bundle in sei-internal-skills. The ticket is right that a Cursor scout is not reachable. The description points a reader at PLT-1168 for that bundle. ## The close path reclaims a scout sandbox, and one gap stays The default closes the leak for a caller that omits the input on both jobs. Two facts make that true: 1. This file sets `SEIDROID_SCOUTS: ${{ inputs.scouts }}` as step env on `Drive session + collect verdict`, with no mode condition, so a close run carries the same value a review run does. 2. The driver's `--close` branch deletes each parsed scout session before the review's own, best effort, and warns per scout that it could not reclaim. One gap stays, and the description names it. The driver derives a scout's session key from the scout NAME, not from the agent (`ScoutRunKey(repo, pr, name)` in `internal/review/scout.go`). A caller that passes `scouts` on the review job and omits it on the close job now gets the default on close. Close then deletes the sessions named `codex` and leaves the configured scout's sandbox running. Defaulting does not fix that case; passing the same value on both jobs does. A leaked scout is also a warning, not a failure, so the close job stays green through it. ## A failing scout already cannot fail the review Verified, and I add no machinery. `gatherScouts` bounds the scouts with their own context deadline and collects a result per slot. `runScout` turns every exit code into a note through `scoutNote`, and a `recover` guard turns a panicking scout into a note as well. The review then runs with fewer readers. Both are in `sei-agent-driver/cmd/sei-agent-driver/main.go` at `sei-agent-driver/v0.14.0`. ## The description states the fork gap, and does not assume it away PR #89 (PLT-1156) is open and not merged into `feat/seidroid-review`, so the `allow-tools` description states the gap this branch carries. An explicit `@seidroid review` arrives as an issue_comment in the base repository, which does carry the secrets, and no head-repository check exists in the guard. A member who asks for a review on a fork-originated pull request runs this shell over fork code. The description names PLT-1156 as the control that refuses one. `grep -i fork` on the rebased base `d477b7d3` returns one line, a pre-existing comment, and the file holds no `head.repo` or `base.repo` check. When #89 lands, the last two sentences of that paragraph need the present-tense refusal. ## Three costs of the scout default, beside the value Flipping `scouts` from `''` changes behaviour for every existing caller. The description names what each caller pays, and how to opt out with `scouts: ''`: - A value on the review job that close does not have leaks that scout. - A deployment without the bundle fails a scout on every pull request, and that failure is a note rather than an error. - A caller with no `mode: close` job leaks one scout sandbox per pull request. The description also narrows the inventory claim to what this file can check: "`xreview-scout-codex` is the one scout bundle sei-internal-skills carries today". ## Verification `actionlint` is unchanged against the rebased base `d477b7d3`. It reports four SC2102 findings before and after, at the same four sites. Only the line numbers move, by the description lines this change adds. ``` $ actionlint .github/workflows/seidroid-review.yml # base exit=1 SC2102 x4 $ actionlint .github/workflows/seidroid-review.yml # this branch exit=1 SC2102 x4 $ diff <(grep -o 'SC[0-9]*' before) <(grep -o 'SC[0-9]*' after) identical rule sets ``` The file parses. A round trip through the parser confirms each description folds into the paragraphs I wrote, with `scouts: ''` and `mode: close` intact inside the folded block: ``` $ python3 -c "import yaml; yaml.safe_load(open('.github/workflows/seidroid-review.yml'))" yaml ok ``` `vale` reports no warning on the six paragraphs this change adds. It still reports four long sentences and three passives in the text around them, which this change does not touch. One error remains, from a rule the global configuration applies to every `*.md`: ``` AgenticWriting.Spec-AcceptanceCriteria Spec has no '#### Acceptance Criteria' heading ``` That rule describes a specification. This body is not one, and I did not silence the rule. ## What I did not verify Nothing here ran on a GitHub runner. I read the workflow and the driver source at the pinned tag; I ran no review, no close, and no scout. The ticket offers "the credential is rotating and down-scoped" as a control. I could not check that from this repository or from the driver, because the server mounts that credential through its admission policy. I left the claim out of the description rather than write a control I cannot support. I also did not see the two callers PLT-1151 describes. This repository wires no caller for `seidroid-review.yml`, so I stated the exposure without claiming how many callers take the default. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eview paths A review clones the pull request's code into a sandbox that holds a live App credential and a shell. Where the code comes from a fork, someone outside the organisation wrote it. The guard's `Admit the request` step now compares the head and base repository ids and refuses when they differ. ai-review.yml refuses the comment path in the same words, at lines 250-257 of that file. Both paths, not one. An issue_comment run receives this workflow's secrets, so a collaborator's comment on a fork pull request drove a full review. A fork pull_request run does not receive them by default, and the machine-client check fails such a run -- but a private or internal repository can turn that withholding off, per repository or by organisation policy. Gating the automatic path on that setting would rest the posture on something no caller of this file controls. Two sources, one rule. A pull_request payload already carries both ids, so that path spends no API call. An issue_comment payload carries no head repository, so the API answers there. Both fail closed: only a definite match admits, and an absent id refuses rather than comparing equal to another absent id. The check sits before the once-per-PR gate and reads the guard's shared GATE_TOKEN, so it costs one API call on the comment path and none elsewhere. The two differ in posture on purpose: the gate fails open, where this refuses. `Require the machine-client secret` reads the verdict as well as the parse. `deny` exits 0, so every step after it still runs, and a fork pull_request run holds no secret to find. Without the verdict there, a refusal ended the guard red against a caller misconfiguration that does not exist. An admitted run still fails fast. The check exempts a close, keyed on the caller's mode rather than on the command this guard parsed. Two readers derive those two from one comment body and can disagree: the guard accepts a bare `seidroid review close`, where a caller matching the documented `@seidroid` form routes that same comment as a review. `@seidroid review close` on a fork pull request still reclaims its sandbox. The allow-tools description names this refusal in the present tense. The unrestricted shell it accepts now runs only over code from inside the organisation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a7bc29a to
b8318b6
Compare
|
Rebased onto The conflicts. Header sentence, guard The one that needed a counterfactual. The env conflict looks like a plain "take both". It is not. Resolving it the obvious way keeps this branch's side, since that is the side carrying the new variables. Doing so drops
Not silent, but total. Every automatic review would end the guard red with an unbound-variable error. The row that catches it is The one that would have been silent. The The comment on that block is the part neither draft got right. Three reads now share the grant, and they do not fail the same way, so the comment says so per reader: the once-per-PR gate fails open, and the fork check fails closed. An interaction I did not expect, and am not fixing. A block this workflow left on a fork pull request now stays until a maintainer dismisses it by hand. That is the right way round: the alternative is running an agent over fork code to retract a review. The case is also narrow, and needs a block that only an already-run review could have created. It is in the PR body under its own heading rather than left for someone to find. Caught by not trusting the green. My first pass of that interaction case used
I dropped one sentence beyond the two you named: "GitHub withholds this workflow's secrets from an automatic fork run". That was the third instance of a claim this branch already corrected twice. It holds by default rather than by guarantee. Verification. Cases exercising code that moved under me: the five interaction rows, Not verified: nothing has run in a GitHub runner. |
There was a problem hiding this comment.
The fork-origin check is correctly placed and correctly fails closed: the empty-BASE_REPO_ID guard stops two absent ids comparing equal, a null head repo reads as fork, origin/refusal are assigned on every branch under set -u, and any unexpected MODE value falls in the safe direction. No blocking issues; one non-blocking consequence for fork pull requests that already carry a standing block from this workflow.
Findings: 0 blocking | 2 non-blocking | 1 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- On the comment path the guard now issues two
GET repos/$REPO/pulls/$PRcalls three lines apart (the fork check at line 680 withGATE_TOKEN, the skip-label check at line 706 withGH_TOKEN). When an App is configured the two tokens are the same value, so it is literally the same request twice. Not worth restructuring given the deliberately different failure modes (fork fails closed, label fails open on no-App), but worth noting if the guard grows a third read of the same object. - 1 suggestion(s)/nit(s) flagged inline on specific lines.
| fi | ||
| case "$origin" in | ||
| same) ;; | ||
| fork) deny "$refusal" ;; |
There was a problem hiding this comment.
[suggestion] This refusal also permanently strands a standing block on any fork pull request this workflow has already reviewed.
The only thing that withdraws a CHANGES_REQUESTED left by this workflow is a later review run (the dismissal loop at line ~1721), and the standing-block exemption that exists specifically to let that run happen (line 744) is evaluated after this check. So once the fork check denies, neither a push nor an explicit @seidroid review can reach the withdrawal.
That matters for two populations that exist today:
- Repositories with "Send secrets and variables to workflows from fork pull requests" enabled — the exact configuration this PR targets — where automatic fork reviews have been running and leaving blocks.
- Any repository at all, via the comment path: before this change nothing refused
@seidroid reviewon a fork pull request, so a member could produce a block on one.
After merge those blocks stand forever and need a repository admin to dismiss by hand, which the input doc at line 356 already warns is restricted on a protected branch.
Moving the check later is not the answer — that would run the agent over fork code, defeating the point. Two cheaper options: mention the manual dismissal in the refusal text so the notice tells the reader what to do, and/or note the one-time sweep in the rollout section of this file alongside the divergence-from-ai-review.yml note.
… and identity (#93) Three defects in the guard's `Admit the request` step, shipped together because all three edit that one step. **PLT-1169**, **PLT-1160** and **PLT-1149**. - **PLT-1169** — the skip-label check keyed on `$COMMAND`, the guard's own parse of the comment body. `inputs.mode` decides whether a review runs. The two grammars 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 a `command` output that nothing reads, which leaves one grammar in the guard. - **PLT-1160** — the label read used the App token alone, so `ai: skip-review` did nothing for a caller with no App. It now reads under `GATE_TOKEN`, and it fails closed. - **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. Review round two adds one fix and one wording change: - **The team check no longer gates `mode: close`.** It did, on the base as well as on the first revision of this PR. Its two neighbours exempt a teardown on purpose, and it now does too. - **The refusal a no-App caller meets on the comment path names its cause, its fix, and what still works.** The same fact now sits in the `allowed-team` input description and the `SEIDROID_APP_ID` secret description. Review round three fixes three more: - **The `review` job's comment matched the old gating.** It said a comment close goes through the same team gate a review does. The exemption made that false. - **`pull-requests: read` is now load-bearing, and the PR body did not say so.** See "The rollout dependency" below. - **The label refusal named a route that cannot work.** It told a no-App caller 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. The `close-*` rows of the table below are the evidence. | # | check | gates a close? | correct? | |---|---|---|---| | 1 | the command parsed at all (`$PARSED`) | **yes** | **yes — keep.** It answers "was this comment a command", not "which command". The guard's whole-line grammar is the documented access control. A caller's `contains()` filter is only a pre-filter. `close-not-a-command` refuses prose that quotes the command, and should. | | 2 | draft | only on a pairing the job `if` blocks | the job condition pairs `pull_request` with `mode: review`, so `EVENT_NAME=pull_request` implies `MODE=review`. `close-auto-draft` refuses, and is unreachable while that condition holds. | | 3 | team membership | **was yes — now no** | **the defect. Fixed here.** `close-team-nonmember` pins the widening: the same actor and state that `team-explicit-nonmember` refuses for a review now reaches a close. | | 4 | fork origin | no (`$MODE != close`) | already correct, from #89 | | 5 | skip-review label | no (`$MODE != close`) | correct after PLT-1169 | | 6 | once-per-PR verdict | only on a pairing the job `if` blocks | same argument as the draft check; `close-auto-prior-verdict` is unreachable while it holds | Exactly 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_request` path, and only the job `if` keeps that pairing from arising. Anyone who widens that condition has to revisit both. **This is a pre-existing defect, not one PLT-1149 introduced.** On the base, `close-team-nonmember`, `close-team-malformed` and `close-team-read-fails` all refuse the teardown. Both existing callers set `allowed-team`, so both carry the 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: read` is now load-bearing On 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 reads repository 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_TOKEN` permissions therefore loses **every automatic review on a private repository**, where the old code reviewed fine. That default grants `contents`, `packages` and `metadata` read, and no `pull-requests`. My earlier caller analysis checked `allowed-team` and the App secrets. 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: | caller | job | mode | `permissions:` | covers `pull-requests: read`? | |---|---|---|---|---| | `sei-load` | all three | review, close, close | `contents: read, pull-requests: write, checks: write, issues: write` | yes — `write` subsumes `read` | | `sei-internal-skills` | all three | review, close, close | `contents: read, pull-requests: write, checks: write` | yes | **Neither breaks.** Only the guard's `permissions:` comment implied the requirement 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-skills` grants no `issues:` scope at any call site, while the guard job 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 `none` and the gate's comment read 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: 1. **The neighbour above it already fails closed on the same read.** The fork check that landed in #89 reads `repos/{owner}/{repo}/pulls/{n}` under `GATE_TOKEN` and refuses when it cannot place the pull request. A label check that admits on that same failed read would give two answers to one API error. 2. **The costs are asymmetric, as PLT-1160 frames them.** A refusal costs one 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. 3. **A teardown is never affected.** `mode: close` skips the check, so a failed label 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: | endpoint | fine-grained permission | covered by the guard's grants | |---|---|---| | `GET /repos/{owner}/{repo}/pulls/{pull_number}` | at least one of "Pull requests" read **or** "Contents" read | yes — the job grants `pull-requests: read` | | `GET /repos/{owner}/{repo}/issues/{issue_number}/labels` | at least one of "Issues" read **or** "Pull requests" read | yes, though the guard reads labels off the pulls endpoint and never calls this one | | `GET /orgs/{org}/teams/{slug}/memberships/{user}` | "Members" **organization** permissions (read) | **no** — the workflow `permissions:` key has no `members` scope, so a `GITHUB_TOKEN` cannot carry it | The third row is why the team check keeps `GH_TOKEN` and gains no fallback: the App identity is the only identity that can answer it. The `GATE_TOKEN` comment now 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_TOKEN` permissions passed from the caller workflow can be only downgraded (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 `parse` and `Admit the request` steps out of the YAML with PyYAML — `jobs.guard.steps[]` — and runs each under `bash`. A `gh` stub on `PATH` serves fixture JSON through the step's **own** `--jq` filter and the real `jq`. The harness resolves every `${{ }}` in both steps' `env:` blocks from the workflow file. It **hard-errors on 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: `5f5fd78` to `5d06528` mid-task, then `41ee3ff` (#90), then `30f5c09` (#95). Every case below comes from a fresh extraction of the rebased file. `#95` moved `driver-version` to `v0.15.0`. Its hunks land at lines 99, 106 and 1035+; the first hunk in this diff is at 109. The `guard` job is byte-identical between `41ee3ff` and `30f5c09`, dumped and diffed the same way as before. The rebase onto `41ee3ff` reported no conflict, so I checked it rather than trusted it. The `guard` job is **byte-identical** between `5d06528` and `41ee3ff` — dumped and diffed. #90's hunks land at lines 1151+ and 2292+, clear of every hunk in this diff. #90 also hoisted `FINDING_MARKER` into the workflow `env:` block. The harness now exports **every** workflow env key rather than the one it used to name, so a later hoist reaches these scripts the way it does on a runner. `admit` as the step wrote it, base `5d06528` against this branch: ``` | case | scenario | base | this PR | |------------------------------|-------------------------------------------------------------|--------|--------| | divergence-labelled | body parses close, caller sends review, labelled | true | false | | divergence-bare | same body, no label | true | true | | divergence-fork | same body, fork-originated | false | false | | close-labelled | teardown, labelled | true | true | | close-label-read-fails | teardown, pulls read fails | true | true | | close-body-review | body parses review, caller sends close, labelled | false | true | | comment-app-labelled | App set, labelled | false | false | | comment-app-bare | App set, no label | true | true | | comment-app-read-fails | App set, pulls read fails | false | false | | comment-noapp-labelled | no App, labelled, comment path | true | false | | comment-noapp-bare | no App, no label, comment path | true | false | | comment-halfapp-bare | half a credential, comment path | false | false | | team-omitted-member | omitted, sei-core member | true | true | | team-omitted-nonmember | omitted, not an active member | true | false | | team-omitted-unknown | omitted, membership unreadable | true | false | | team-explicit-empty | caller passes allowed-team: '' | true | false | | team-explicit-member | existing caller, sei-core member | true | true | | team-explicit-nonmember | existing caller, not a member | false | false | | team-malformed | allowed-team with no slash | false | false | | not-a-command | prose that mentions the command | false | false | | auto-noapp-labelled | no App, labelled | true | false | | auto-noapp-bare | no App, no label | true | true | | auto-app-labelled | App set, labelled | false | false | | auto-app-bare | App set, no label | true | true | | auto-halfapp-labelled | half a credential, labelled | false | false | | auto-halfapp-bare | half a credential, no label | false | true | | auto-noapp-read-fails | caller grants no pull-requests: read | true | false | | auto-team-set-noapp | team set, automatic path skips it | true | true | | auto-draft | draft | false | false | | auto-fork | fork-originated | false | false | | auto-prior-verdict | a verdict already stands | false | false | | auto-standing-block | a block from this workflow stands | true | true | | label-other | a different label | true | true | | label-superstring | a label the skip label is a prefix of | true | true | | label-key-absent | the payload carries no labels key | true | true | | label-input-empty | skip-review-label passed empty | true | true | | close-team-omitted-noapp | close: team omitted, no App | true | true | | close-team-nonmember | close: commander not on the team, team set | false | true | | close-nonmember-team-default | close: commander not on the team, team defaulted | true | true | | close-team-empty | close: allowed-team passed empty | true | true | | close-team-malformed | close: allowed-team with no slash | false | true | | close-team-read-fails | close: membership read fails | false | true | | close-fork | close: fork-originated | true | true | | close-not-a-command | close: prose, the guard's grammar refuses | false | false | | close-auto-draft | close on pull_request, draft (job `if` blocks this pairing) | false | false | | close-auto-prior-verdict | close on pull_request, prior verdict (job `if` blocks this) | false | false | ``` Thirteen verdicts change. Each one is a ticket or a review finding asking for it: | case | change | why | |---|---|---| | `divergence-labelled` | admit → deny | PLT-1169, the reachable bypass | | `close-body-review` | deny → admit | PLT-1169 in the other direction: the guard refused a teardown whose body reads as a review | | `auto-noapp-labelled` | admit → deny | PLT-1160, the label now bites with no App | | `auto-noapp-read-fails` | admit → deny | PLT-1160, the fail-closed posture | | `auto-halfapp-bare` | deny → admit | the label now reads under `github.token`, so the half-credential refusal loses its premise | | `team-omitted-nonmember` | admit → deny | PLT-1149, the default | | `team-omitted-unknown` | admit → deny | PLT-1149, an unreadable membership | | `team-explicit-empty` | admit → deny | PLT-1149, empty denies | | `comment-noapp-labelled` | admit → deny | the team check refuses first — see below | | `comment-noapp-bare` | admit → deny | the team check refuses first — see below | | `close-team-nonmember` | deny → **admit** | review finding: the team check stranded a teardown | | `close-team-malformed` | deny → **admit** | same | | `close-team-read-fails` | deny → **admit** | same | ### A no-App caller on the comment path: accepted, and the refusal says why **The automatic `pull_request` path is unaffected.** A reader will assume that half broke, so it goes first. `auto-noapp-bare` admits. `auto-noapp-labelled` refuses 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-team` is non-empty by default. The team 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 is not new coupling: any caller that sets `allowed-team` has it today. The refusal now reads: > this run holds no App identity, so it cannot read membership of > sei-protocol/sei-core; denying. Pass SEIDROID_APP_ID and > SEIDROID_APP_PRIVATE_KEY to this workflow. An automatic pull_request review > and @seidroid review close do not reach this check 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: > could not read the labels on OWNER/REPO#N, so ai: skip-review cannot be ruled > out; not reviewing. Grant pull-requests: read on the calling job, or pass > SEIDROID_APP_ID and SEIDROID_APP_PRIVATE_KEY Cause, fix, and what still works. The `allowed-team` input description and the `SEIDROID_APP_ID` secret description carry the same fact, because the person configuring 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 set `allowed-team: ''`**. The second one does not work. PLT-1149 makes an empty `allowed-team` deny, and the same review 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 out exists, 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-load` and `sei-protocol/sei-internal-skills` both pass `allowed-team: 'sei-protocol/sei-core'` on their review and close jobs, and both configure the App. `team-explicit-member`, `team-explicit-nonmember`, `comment-app-*` and `auto-app-*` are unchanged, so their behaviour holds when they bump their pin. Their third job, `seidroid-review-reclaim`, omits `allowed-team`. It fires a `pull_request` event with `mode: close`. The guard's `if` does not match that pair, 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 `SC2102` before and after, at the same offsets inside the extracted scripts. ``` $ actionlint -oneline base.yml base.yml:1474:9: shellcheck reported issue in this script: SC2102:info:44:14: Ranges can only match single chars (mentioned due to duplicates) [shellcheck] base.yml:1474:9: shellcheck reported issue in this script: SC2102:info:45:14: Ranges can only match single chars (mentioned due to duplicates) [shellcheck] base.yml:1978:9: shellcheck reported issue in this script: SC2102:info:207:16: Ranges can only match single chars (mentioned due to duplicates) [shellcheck] base.yml:1978:9: shellcheck reported issue in this script: SC2102:info:208:16: Ranges can only match single chars (mentioned due to duplicates) [shellcheck] $ actionlint -oneline .github/workflows/seidroid-review.yml .github/workflows/seidroid-review.yml:1468:9: shellcheck reported issue in this script: SC2102:info:44:14: Ranges can only match single chars (mentioned due to duplicates) [shellcheck] .github/workflows/seidroid-review.yml:1468:9: shellcheck reported issue in this script: SC2102:info:45:14: Ranges can only match single chars (mentioned due to duplicates) [shellcheck] .github/workflows/seidroid-review.yml:1972:9: shellcheck reported issue in this script: SC2102:info:207:16: Ranges can only match single chars (mentioned due to duplicates) [shellcheck] .github/workflows/seidroid-review.yml:1972:9: shellcheck reported issue in this script: SC2102:info:208:16: Ranges can only match single chars (mentioned due to duplicates) [shellcheck] ``` Grouping the parse step's three `>> "$GITHUB_OUTPUT"` writes is what keeps that set unchanged. Removing the `command=` classification left three adjacent redirects, which raised a new `SC2129`; the `{ … } >> file` form matches the shape the same step's `pull_request` branch 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 past the force-push. It served the pre-fix tree and produced a table that disagreed with the working tree. `git ls-remote` said `f7add4a`; the tracking ref said `9c56ab5`. The numbers above come from a re-fetched ref, and the file behind them is byte-identical to the working tree (`cmp`). Also checked, on the changed file: - YAML parses (PyYAML), both jobs present. - `shellcheck -s bash` on the extracted `Admit the request` script: clean. - Every `$VAR` in every guard `run:` script resolves: a step, job or workflow `env:` key declares it, the script assigns it, or the script reads it as `${VAR:-}`. Every `env:` key has a reader. That is the `set -u` check. Dropping `COMMAND`, `APP_ID_PRESENT` and `APP_KEY_PRESENT` must not strand one. ## What I did not verify - Nothing ran on a GitHub runner. Every result above comes from the extracted shell against a stub. - Whether GitHub errors or silently downgrades when a called workflow requests a permission its caller did not grant. - The real GitHub API's exact failure shapes. The stub models an authentication failure and a non-zero `gh api` exit; it does not model a partial page or a rate limit. - Pagination. The label read fetches one pull request, so `--paginate` does not apply, 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 - Both callers' `seidroid-review-reclaim` job carries a comment calling `allowed-team` "optional there (default '')". The parenthesis goes stale with this PR. The behaviour does not change, because that job never reaches the guard. Recorded here as a follow-up in `sei-load` and `sei-internal-skills`; I did not edit either repository. - This workflow has no README documenting its inputs the way `ai-review.yml` does. The input descriptions in the file are the only reference, and three of them changed here. ## Corrections to the three tickets - **PLT-1160** says the half-configured caller "already denies before this check". It denies *inside* the check, as its `elif` branch. That branch rests on one premise: half a credential mints no token, so nothing can read the label. The fallback to `github.token` ends that premise, so this PR drops the branch. The `Report a half-configured reviewer identity` step still names the missing half. - **PLT-1149** says to "keep the existing behaviour that an unset team on the comment path refuses". The existing behaviour *admits*: an empty `allowed-team` skipped the check. This PR implements the refusal the sentence asks 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](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
A review clones the pull request's code into a sandbox that holds a live App credential and a shell. Where that code comes from a fork, someone outside the organisation wrote it. The guard's
Admit the requeststep now compares the head and base repository ids, and refuses when they differ. It refuses on both paths.What changed
.github/workflows/seidroid-review.yml, +105/-33. Rebased onto5f5fd78, which is#86and#91.The check, in
Admit the request, between the team-membership check and the skip-label check:The signals.
MODE: ${{ inputs.mode }},HEAD_REPO_IDandBASE_REPO_IDin the step'senv:. The gate keys on the caller's routing, not on this guard's own re-parse of the comment body.The token.
GATE_TOKEN, which#86added to the same step and this check now shares. Anissue_commentpayload carries nopull_request.head.repo, so the API answers there, and the read must work whether or not a caller configures an App.The permission.
#86already grants the guardpull-requests: readandissues: read. This check adds no grant. It rewrites the comment on that block, because three reads now share it and they do not fail the same way.The verdict gate.
Require the machine-client secretnow readssteps.admit.outputs.admit == 'true'as well as the parse. See below.Three comments that stated something this change makes false. Listed at the end.
Two sources, one rule
A
pull_requestpayload already carries both ids, so that path spends no API call. Anissue_commentpayload carries no head repository, so the API answers there.The payload branch tests the base id first. Comparing two absent ids makes them equal, which would admit a fork on a payload that carried no signal at all. A present base id with an absent head id, the deleted-fork case, falls to
fork.How the incumbent words its refusal
.github/workflows/ai-review.ymllines 250-257:Two things carry over. First the sentence:
explicit re-reviews are disabled for fork-originated pull requests, word for word, in lower case, with; not reviewing $REPO#$PRafter it. The draft and skip-label denies in this file already use that shape. Second the comparison: repository ids, not names, so a rename does not read as a fork. A null head repository reads as a fork.The refusal wording differs by path on purpose. The incumbent's sentence is accurate on the comment path. Nobody asked explicitly on the automatic path, so that path says
$REPO#$PR is fork-originated; not reviewing it.Divergence from the incumbent, recorded on purpose
ai-review.ymlrefuses forks on the explicit path only (!isAutomatic && isFork) and still reviews a fork pull request automatically. This refuses both paths. That is a deliberate posture change, not a port, and it is worth being exact about what it costs.Under the default, it costs nothing. A fork
pull_requestrun receives no secrets, soRequire the machine-client secretfails it today. This change turns that hard failure into a clean refusal.Under one configuration it does remove a working behaviour, and that removal is the objective. A private or internal repository can enable one Actions setting: "Send secrets and variables to workflows from fork pull requests". A repository owner or an organisation policy sets it. There, a fork
pull_requestrun receivesOMNIGENT_MACHINE_CLIENT_SECRET, passes the machine-client check, and drives an agent over fork code. That is the exposure this ticket exists to close. Calling it lost coverage would be reading it backwards: it is outside code beside a live credential, and the incumbent has the same gap.Fail closed, unlike the neighbours
The step runs under
set -uo pipefailwith no-e. A failed command substitution does not stop the step; it leaves the variable empty and carries on. Thecasetherefore admits onsamealone. Every other value refuses, including empty.That is the opposite of the checks around it:
The code states that reason, directly above the check.
The refusal names a cause.
ghwrites its own error to the step log, so a 403 or a 404 appears immediately above the notice:A refused fork ends green, not red
denyexits 0, so every step afterAdmit the requeststill runs.Require the machine-client secretread the parse alone. A refused forkpull_requestrun therefore reached it, found no secret, and ended the guard red. That pointed at a caller misconfiguration that does not exist, and contradicted the notice the gate had just written. The step now reads the verdict too.Evaluated against both revisions, with the step's own script run when the condition holds:
should_run == 'true'::error::OMNIGENT_MACHINE_CLIENT_SECRET is not set…should_run == 'true' && admit == 'true'The fail-fast survives where it belongs:
guard.should_runpull_request, no secrets, refusedThe review job skips either way, so only the guard's colour changes.
Audit of the other steps.
Report a half-configured reviewer identityalso keys on the parse alone. It stays that way deliberately. #88 madeAdmit the requestdeny when a caller sets half an App credential. That warning is what explains the deny. Gating it onadmitwould suppress the diagnostic exactly where a reader needs it. No other guard step keys offshould_run.The App stays optional
GATE_TOKENprefers the App identity and falls back togithub.token, so a caller that configures noSEIDROID_APP_IDstill reaches the read. A same-repository pull request admits there, and a fork refuses. The secret'srequired: falsecontract holds.The name sits apart from
GH_TOKENon purpose. The team check has no such fallback: reading an organisation's teams needs an identity that can see them, andGITHUB_TOKENcannot.The close path still runs
@seidroid review closeon a fork pull request still reclaims its sandbox. A close is the only thing that reclaims one: no lifetime cap and no sweep does it instead. The comment-path close depends on the guard's verdict — the review job requiresneeds.guard.outputs.should_run == 'true'for everyissue_commentrun — so a deny would block the reclaim.The gate keys on
$MODE, not on$COMMAND. Two readers derive those two from one comment body, and they can disagree. This guard's grammar accepts a bareseidroid review closewith no@. A caller matching the documented@seidroid review closeform routes that same comment asmode: review.Keying on
$COMMANDtherefore skipped the fork check on a comment the caller had routed as a real review. That is the bypass this PR exists to close.$MODEdecides what the review job does. Nothing risky runs when the mode is close, so the exemption stays safe both ways.Three comments this change corrects
Two of them asserted that GitHub withholds secrets from a fork
pull_requestrun. Both rested a safety argument on it. That holds by default, not by guarantee.pull_requestbranch, partly on the same withholding. It now points at this gate.Both paths. That sentence already ran to 35 words, so I split it into four rather than adding a clause.Verification
Rebased onto
feat/seidroid-reviewat5f5fd78(#91).actionlint1.7.12 against that base and against this branch:YAML parses:
python3 -c "import yaml; yaml.safe_load(open('.github/workflows/seidroid-review.yml'))"returns clean.yaml.safe_loadextracts the step's script from the YAML. A harness runs it underbash, with aghstub onPATHthat refuses when it receives no token, the wayghitself does.closeclose, caller sentmode: reviewmode: closeclose, callermode: reviewpull_request+ fork payload#42 is fork-originatedpull_request+ same-repo payloadpull_request+ null head repo idpull_request+ no ids at allpull_request+ fork + draftpull_request+mode: closeclosecloseReal
jqanswered five payload shapes: same ids tosame; different ids tofork;head.repo: nulltofork;head.repoabsent tofork; an error body tofork.API cost, counted by the stub: one read on the comment-review path, zero on every other path.
Not verified from here
Nothing here has run in a GitHub runner. The harness proves three things: the shell logic, the jq mapping, and the step conditions evaluated the way GitHub would for this expression shape. It does not prove that
github.tokenwithpull-requests: readanswersrepos/{repo}/pulls/{n}in a real run.No repository of mine enables that Actions setting. I have therefore not observed a fork
pull_requestrun receiving secrets. That setting's existence and effect come from review, not from measurement. Not depending on the default is sound either way.Where this check meets the once-per-PR gate
#86's gate sits after this one, so the ordering matters and the harness covers it.synchronize, same-repo, standing blocksynchronize, same-repo, block read failssynchronize, fork, standing blockThe last row is a consequence worth stating.
#86runs a review on a pull request carrying a standingCHANGES_REQUESTEDfrom this workflow, because the withdrawal lives inside a review. On a fork this check refuses that review, so such a block stays until a maintainer dismisses it by hand.That is the right way round. The alternative is running an agent over fork code to retract a review. The case is also narrow. It needs a block this workflow left on a fork pull request, and only a review that already ran could have created one.