Skip to content

feat(seidroid-review): default to a codex scout, and record two accepted postures - #91

Merged
bdchatham merged 1 commit into
feat/seidroid-reviewfrom
feat/scout-default-and-recorded-decisions
Sep 6, 2026
Merged

feat(seidroid-review): default to a codex scout, and record two accepted postures#91
bdchatham merged 1 commit into
feat/seidroid-reviewfrom
feat/scout-default-and-recorded-decisions

Conversation

@bdchatham

@bdchatham bdchatham commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

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

@cursor

cursor Bot commented Sep 6, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changing the scouts default affects every caller that omits the input (extra model quota, scout sandboxes, and close-job parity). No auth logic changes, but documented fork-review shell exposure remains until PLT-1156.

Overview
Default reviews now run a Codex scout before the main review: the scouts workflow input changes from empty to codex=xreview-scout-codex, so callers that omit it get a second independent reading (main agent + scout) via SEIDROID_SCOUTS. The input description is expanded with failure/degradation behavior, when to pass scouts: '', and that review and mode: close must use the same scouts value to avoid leaving scout sandboxes running.

Documentation-only updates on adjacent inputs record accepted operator decisions: omnigent-base-url notes the default is the dev seigent deployment and its exposure; allow-tools records acceptance of Bash,Read (vs tighter ai-review scoping) and calls out the fork + @seidroid review path where secrets can still run shell over fork code until PLT-1156 lands.

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

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Documentation-heavy change to seidroid-review.yml with one behaviour change: scouts now defaults to codex=xreview-scout-codex. The scout default and its close-path reclamation check out against the workflow source, but the new allow-tools risk-acceptance paragraph rests on a fork guard this branch does not carry.

Findings: 0 blocking | 4 non-blocking | 2 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • This repository wires no caller for seidroid-review.yml, so the new default has not run end to end anywhere in-tree and the PR body confirms nothing ran on a runner. Worth confirming on the first live run that the scout agent resolves on the configured omnigent-base-url and that a mode: close run actually reclaims the scout session, since a silent failure on either shows up only as a note inside a review.
  • xreview-scout-codex is the only scout bundle that exists` is an absolute claim about another repository's inventory that this file cannot check and that PLT-1168 is expected to falsify. Consider phrasing it as what is true today rather than as a fact about the set.
  • 2 suggestion(s)/nit(s) flagged inline on specific lines.

Comment thread .github/workflows/seidroid-review.yml Outdated
that capability.

The grant covers same-repository code. A review runs on the repository
the pull request is on. The guard refuses an explicit request on a

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[suggestion] This states a control the branch does not carry. fork appears nowhere in this file except this new prose and the pre-existing comment at line 340; there is no head.repo / full_name check anywhere in the guard. The issue_comment arm of the guard condition (line 343) admits any OWNER/MEMBER/COLLABORATOR comment on any pull request, fork-originated or not, and issue_comment runs in the base-repo context with this workflow's secrets available — so the explicit-request path is precisely the one the next sentence's "GitHub withholds this workflow's secrets from an automatic fork run" does not cover.

The result is that the risk-acceptance paragraph for an unrestricted Bash grant is supported by one unimplemented control and one that only applies to the other trigger. A future reader weighing whether to widen or narrow this list, as the paragraph invites, will read a boundary that isn't there.

Either merge #89 (PLT-1156) first, as the PR body suggests, or state this in future tense — e.g. "PLT-1156 will refuse an explicit request on a fork-originated pull request; until it lands, a member's @seidroid review on a fork-originated pull request runs the shell over fork code."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in b2a86f9. #89 is still open, so the paragraph now states the gap
rather than the control:

Fork code sits outside it, and one path still reaches it. A review runs on
the repository the pull request is on, and GitHub withholds this workflow's
secrets from an automatic fork run. An explicit @seidroid review arrives as
an issue_comment in the base repository, which does carry the secrets.
PLT-1156 is the control that refuses such a request on a fork-originated pull
request. It is not in this file yet, so a member who asks for one runs this
shell over fork code. Weigh that before you widen or narrow this list.

I confirmed your reading against the rebased base: grep -i fork on
d477b7d3 returns one line, the pre-existing comment, and no head.repo or
base.repo check exists anywhere in the file.

When #89 lands, that paragraph needs its last two sentences replaced with the
present-tense refusal.

required: false
type: string
default: ''
default: 'codex=xreview-scout-codex'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[suggestion] Flipping this default from '' changes behaviour for every existing caller of this reusable workflow, and two consequences are worth naming next to the value:

  • It couples the workflow to a bundle inventory it cannot verify. omnigent-base-url is caller-settable, so a caller pointing at a deployment without xreview-scout-codex now gets a failed scout on every pull request. Per the description above that degrades to a note rather than an error, which means it is invisible unless someone reads the review body or the logs — the same failure mode the agent-id description above documents for the review's own agent.
  • Every review now holds a scout sandbox that only the close path reclaims. A caller that wired the review job but no mode: close job previously created zero scout sandboxes and now leaks one per pull request, with nothing else able to reclaim it (no lifetime cap, no sweep, per the comment at line 517).

The existing text covers the asymmetric review/close case well; adding one line on how to turn scouts off (scouts: '') and what a caller on another deployment should do would make both reachable from the same place.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in b2a86f9. Both consequences now sit beside the value, with the escape
hatch:

That note is the only signal, so a bundle absent from a deployment fails
quietly on every pull request.

Set scouts: '' to review on one model. Do that on a deployment that does
not carry the bundle above, and on a caller that wires no mode: close job.
A scout holds a sandbox of its own, and close is the only thing that
reclaims one. A caller with no close job leaks one sandbox per pull request.

I also softened the inventory claim, which asserted more than this file can
check: "xreview-scout-codex is the one scout bundle sei-internal-skills
carries today".

The asymmetric review/close paragraph is unchanged.

…ted postures

Every review now reads on two models. `scouts` defaults to
`codex=xreview-scout-codex`, which is the one scout bundle
sei-internal-skills carries today. A Cursor bundle is tracked as PLT-1168
(PLT-1163).

The default is written as `name=agent`, not as the bare bundle name. The
driver's parseScouts refuses an entry with no `=` and exits ExitConfig, so
a bare value would refuse every review before it starts.

The default also closes the scout-sandbox leak for a caller that omits the
input on both jobs. The drive step passes SEIDROID_SCOUTS in either mode,
and the driver's --close branch deletes each parsed scout session before
the review's. Three costs land on a caller, so the description names each
one 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 quietly on every pull request, and a caller with no close
job leaks one sandbox per pull request.

The `allow-tools` and `omnigent-base-url` defaults do not change. Their
descriptions state the exposure each one accepts, who accepted it, and
what a caller should weigh before it changes the value (PLT-1157,
PLT-1151). The `allow-tools` description states the fork gap this branch
carries: an explicit review arrives as an issue_comment in the base
repository with the secrets, and PLT-1156 is the control that refuses one
on a fork-originated pull request.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@bdchatham
bdchatham force-pushed the feat/scout-default-and-recorded-decisions branch from fce934e to b2a86f9 Compare September 6, 2026 21:11

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

One behaviour change (the scouts input now defaults to codex=xreview-scout-codex) plus prose recording two accepted postures. The change is coherent and self-consistent with the rest of the file, but flipping scouts from opt-in to on-by-default silently changes cost, wall-clock and sandbox lifecycle for every existing caller that never edits its call.

Findings: 0 blocking | 6 non-blocking | 2 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • timeout-minutes (default 45) and its description were not revisited. The scout runs inside the same Drive session + collect verdict step and therefore under the same job cap, so the default now spends one budget on two sequential sessions. Worth confirming the 45-minute default still leaves headroom above SEIDROID_RUN_DEADLINE_S with a scout in the path, since the whole point of the cap's description is that the driver reports the timeout before the runner kills the job.
  • Rollout asymmetry across the merge: a pull request whose review ran before this merges (old default '', no scout session) gets a close afterwards that reads the new default and tries to delete a codex session that never existed. The PR body asserts the driver's --close scout deletion is best-effort with a per-scout warning, which would make this a spurious warning rather than a failure — worth confirming, because the close step turns a non-zero exit into ::error::close failed ... the session and its sandbox are still running, which would be misleading here.
  • The paragraph at lines 226-231 says close "deletes the default name, and the configured scout keeps its sandbox running". That only holds when the caller's scout name differs from codex; a caller following the documented codex=... convention would in fact have its session reclaimed. Minor, but the sentence reads as unconditional.
  • 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] The allow-tools default of Bash,Read grants an unrestricted shell, and the guard job (.github/workflows/seidroid-review.yml, the if: at ~line 369) has no fork check on the issue_comment path: an OWNER/MEMBER/COLLABORATOR commenting @seidroid review on a fork-originated pull request runs the workflow in the base repository, where secrets are present, and the agent then executes an unrestricted shell over fork-controlled code with its own gh credentials. This PR documents the gap and names PLT-1156 as the control but does not introduce or close it; the automatic pull_request path is unaffected because GitHub withholds secrets there.

required: false
type: string
default: ''
default: 'codex=xreview-scout-codex'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[suggestion] This flips scouts from opt-in to on-by-default for every existing caller, and the two consequences land on callers that change nothing.

A caller that wired only a review job and no mode: close job previously leaked no scout sandbox (default '' produced none). After this it leaks one per pull request, indefinitely, because close is the only thing that reclaims a sandbox. The remedy documented below — "Set scouts: ''" — requires the caller to notice this default changed and edit their call; nothing in the workflow detects the missing close job or warns.

It also silently doubles model spend per review for every caller on the default.

Both are defensible as a deliberate decision, but consider whether the safer shape is landing PLT-1168's caller-side guidance first, or gating the default (e.g. keep '' here and set the two-model value explicitly in the merge-path callers) so the sandbox-lifecycle obligation is taken on knowingly rather than inherited.

tracks a Cursor bundle. A scout that fails costs the review that reading
and nothing else. The driver turns each failure into a note, hands it to
the review, and the review reports with fewer readers. That note is the
only signal, so a bundle absent from a deployment fails quietly on every

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[suggestion] "That note is the only signal, so a bundle absent from a deployment fails quietly on every pull request" — this is now the default path, so a deployment without xreview-scout-codex degrades to a single-model review on every pull request with no operational signal outside the review body.

The build step already has a contract check that fails fast on a driver whose flag surface this file cannot drive, on the reasoning that a misconfiguration should cost an install rather than a review. The same reasoning applies here: if the driver reports scout failures in a machine-readable way (a field in check.json, or a distinguishable exit path), the drive step could emit a ::warning:: so a missing bundle shows up in the Actions annotation rather than only in prose a reader may not compare across runs. If the driver exposes nothing to hang that on today, a ticket reference for it would be worth adding here.

@bdchatham
bdchatham merged commit 5f5fd78 into feat/seidroid-review Sep 6, 2026
12 of 17 checks passed
@bdchatham
bdchatham deleted the feat/scout-default-and-recorded-decisions branch September 6, 2026 21:22
bdchatham added a commit that referenced this pull request Sep 6, 2026
…ed pull request (#89)

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 request` step
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 onto
`5f5fd78`, which is `#86` and `#91`.

**The check**, in `Admit the request`, between the team-membership check
and the skip-label check:

```sh
if [ "$MODE" != "close" ]; then
  if [ "$EVENT_NAME" = "pull_request" ]; then
    if [ -z "$BASE_REPO_ID" ]; then
      origin=unreadable
    elif [ "$HEAD_REPO_ID" = "$BASE_REPO_ID" ]; then
      origin=same
    else
      origin=fork
    fi
    refusal="$REPO#$PR is fork-originated; not reviewing it"
  else
    origin="$(GH_TOKEN="$GATE_TOKEN" gh api "repos/$REPO/pulls/$PR" \
      --jq 'if .head.repo.id != null and .head.repo.id == .base.repo.id then "same" else "fork" end' \
      || true)"
    refusal="explicit re-reviews are disabled for fork-originated pull requests; not reviewing $REPO#$PR"
  fi
  case "$origin" in
    same) ;;
    fork) deny "$refusal" ;;
    *) deny "could not read where $REPO#$PR comes from, so a fork cannot be ruled out; not reviewing it" ;;
  esac
fi
```

**The signals.** `MODE: ${{ inputs.mode }}`, `HEAD_REPO_ID` and
`BASE_REPO_ID` in the step's `env:`. 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 `#86` added to the same step and this
check now shares. An `issue_comment` payload carries no
`pull_request.head.repo`, so the API answers there, and the read must
work whether or not a caller configures an App.

**The permission.** `#86` already grants the guard `pull-requests: read`
and `issues: 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 secret` now reads
`steps.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_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.

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.yml` lines 250-257:

```js
const isFork = pr.head.repo?.id !== pr.base.repo?.id;
if (!isAutomatic && isFork) {
  core.notice(
    "Explicit re-reviews are disabled for fork-originated pull requests."
  );
```

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#$PR` after 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.yml` refuses 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_request` run receives
no secrets, so `Require the machine-client secret` fails 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_request` run receives
`OMNIGENT_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 pipefail` with no `-e`. A failed command
substitution does not stop the step; it leaves the variable empty and
carries on. The `case` therefore admits on `same` alone. Every other
value refuses, including empty.

That is the opposite of the checks around it:

| check | on an unreadable signal | why |
|---|---|---|
| skip-review label | admits | a convenience, not a control. Being
unable to read it must not stop every review. |
| once-per-PR (#86) | admits | same reasoning: one extra review,
corrected by the next push. |
| **fork origin (this)** | **refuses** | a refused review costs one
retry a person can make. Admitting on a signal nobody could read costs
the sandbox. |

The code states that reason, directly above the check.

The refusal names a cause. `gh` writes its own error to the step log, so
a 403 or a 404 appears immediately above the notice:

```
gh: Resource not accessible by integration (HTTP 403)
::notice::could not read where #42 comes from, so a fork cannot be ruled out; not reviewing it
```

## A refused fork ends green, not red

`deny` exits 0, so every step after `Admit the request` still runs.
`Require the machine-client secret` read the parse alone. A refused fork
`pull_request` run 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:

| revision | condition | step | guard |
|---|---|---|---|
| before | `should_run == 'true'` | runs, exit 1 | RED,
`::error::OMNIGENT_MACHINE_CLIENT_SECRET is not set…` |
| after | `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, so only the guard's colour changes.

**Audit of the other steps.** `Report a half-configured reviewer
identity` also keys on the parse alone. It stays that way deliberately.
#88 made `Admit the request` deny when a caller sets half an App
credential. That warning is what explains the deny. Gating it on `admit`
would suppress the diagnostic exactly where a reader needs it. No other
guard step keys off `should_run`.

## The App stays optional

`GATE_TOKEN` prefers the App identity and falls back to `github.token`,
so a caller that configures no `SEIDROID_APP_ID` still reaches the read.
A same-repository pull request admits there, and a fork refuses. The
secret's `required: false` contract holds.

The name sits apart from `GH_TOKEN` on purpose. The team check has no
such fallback: reading an organisation's teams needs an identity that
can see them, and `GITHUB_TOKEN` cannot.

## The close path still runs

`@seidroid review close` on 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 requires
`needs.guard.outputs.should_run == 'true'` for every `issue_comment` run
— 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 bare `seidroid review close` with no `@`. A caller matching
the documented `@seidroid review close` form routes that same comment as
`mode: review`.

Keying on `$COMMAND` therefore skipped the fork check on a comment the
caller had routed as a real review. That is the bypass this PR exists to
close. `$MODE` decides 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_request` run. Both rested a safety argument on it. That holds by
default, not by guarantee.

- The check's own comment said the automatic path never reaches it. It
now names the default, names the setting that disables it, and states
that the check does not rest on it.
- The guard job's comment justified having no author-association check
on the `pull_request` branch, partly on the same withholding. It now
points at this gate.
- The header block enumerates the gates. It now names the fork refusal,
and says `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-review` at `5f5fd78` (#91). `actionlint`
1.7.12 against that base and against this branch:

```
before: 4 findings, all SC2102
after:  4 findings, all SC2102
diff of the two, normalised for line numbers: identical
```

YAML parses: `python3 -c "import yaml;
yaml.safe_load(open('.github/workflows/seidroid-review.yml'))"` returns
clean.

`yaml.safe_load` extracts the step's script from the YAML. A harness
runs it under `bash`, with a `gh` stub on `PATH` that refuses when it
receives no token, the way `gh` itself does.

| case | verdict |
|---|---|
| fork PR + review comment | DENY, fork refusal |
| same-repo PR + review comment | ADMIT |
| fork PR + `close` | ADMIT |
| fork + body `close`, caller sent `mode: review` | DENY, fork refusal |
| fork + body review, caller sent `mode: close` | ADMIT |
| same-repo + body `close`, caller `mode: review` | ADMIT |
| `pull_request` + fork payload | DENY, `#42 is fork-originated` |
| `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 |
| read fails 404 | DENY, could not read |
| read fails 403 | DENY, could not read |
| read returns nothing | DENY, could not read |
| deleted fork, null head repo | DENY, fork refusal |
| no App, same-repo PR + review comment | ADMIT |
| no App, fork PR + review comment | DENY, fork refusal |
| no App, fork PR + `close` | ADMIT |
| no token at all | DENY, could not read |
| same-repo + `close` | ADMIT |
| automatic, same-repo, not draft | ADMIT |
| automatic, same-repo, draft | DENY, draft, unchanged |
| same-repo + skip label | DENY, label, unchanged |
| half an App credential (#88) | DENY, half-credential, unchanged |
| fork PR + team member | DENY, fork refusal |
| same-repo + non-member | DENY, membership, unchanged |
| parse said no | DENY, unchanged |

Real `jq` answered five payload shapes: same ids to `same`; different
ids to `fork`; `head.repo: null` to `fork`; `head.repo` absent to
`fork`; an error body to `fork`.

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.token` with `pull-requests: read` answers
`repos/{repo}/pulls/{n}` in a real run.

No repository of mine enables that Actions setting. I have therefore not
observed a fork `pull_request` run 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.

| case | verdict |
|---|---|
| automatic, same-repo, verdict already posted | DENY, the gate's own
refusal |
| automatic, fork, verdict already posted | DENY, fork refusal — the
gate is never reached |
| `synchronize`, same-repo, standing block | ADMIT, the gate's
withdrawal path intact |
| `synchronize`, same-repo, block read fails | ADMIT, the gate still
fails open |
| `synchronize`, fork, standing block | DENY, fork refusal |

The last row is a consequence worth stating. `#86` runs a review on a
pull request carrying a standing `CHANGES_REQUESTED` from 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.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant