Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
bdchatham merged 1 commit into from
Sep 6, 2026
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 46 additions & 8 deletions .github/workflows/seidroid-review.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,12 @@ on:
in-cluster ClusterIP Service, which is plain http on port 80: a
credentialed client refuses to be built against it, and the token mint
refuses to send the client secret over it.

This default is a development deployment. Platform changes land there
first, so a change on it can make a review succeed and post nothing.
Brandon Chatham accepts that exposure: no production deployment carries
the seidroid bundle and a provisioned machine client today. Pass a
production URL from a merge-path caller as soon as one exists.
required: false
type: string
default: 'https://seigent.dev.platform.sei.io'
Expand Down Expand Up @@ -202,20 +208,36 @@ on:
scouts:
description: >-
Independent readings to gather before the review, as `name=agent`,
comma-separated. Empty runs the review alone, which is what it did
before scouts existed. Each scout reads the same pull request in its own
comma-separated. Each scout reads the same pull request in its own
session on its own agent bundle, seeing neither the review nor another
scout; the review then verifies their claims against the diff and merges
what holds. A scout naming the review's own agent is refused, as are two
scouts sharing one — neither would be a second opinion. Passed to the driver as
SEIDROID_SCOUTS.
scouts sharing one — neither would be a second opinion. Empty runs the
review alone. Passed to the driver as SEIDROID_SCOUTS.

Two models read every pull request by default. `xreview-scout-codex` is
the one scout bundle sei-internal-skills carries today, and PLT-1168
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

@github-actions github-actions Bot Sep 6, 2026

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.

pull request.

Pass the SAME value on the CLOSE job. Scouts hold sessions of their own,
and close derives which to delete from this value, keyed on the scout
NAME. A caller that omits it on both jobs is safe, because close reads
this same default. A caller that sets it on the review job and omits it
on the close job is not. Close then deletes the default name, and the
configured scout keeps its sandbox running.

Set this on the CLOSE job too. Scouts hold sessions of their own, and
close derives which to delete from this value: unset there, every scout
sandbox is left running with nothing able to reclaim it.
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.
required: false
type: string
default: ''
default: 'codex=xreview-scout-codex'

@github-actions github-actions Bot Sep 6, 2026

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.

@bdchatham bdchatham Sep 6, 2026

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.

@github-actions github-actions Bot Sep 6, 2026

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.

claude-model:
description: >-
Model to answer the review on, substituting for the one the agent's spec
Expand Down Expand Up @@ -263,6 +285,22 @@ on:
outside one does, and a recorded run had exactly that refused and spent
three extra tool calls recovering. The diff now stages into the working
directory, so this grant is the belt to that braces.

Brandon Chatham accepts the unrestricted shell. The review builds and
tests the tree where that is straightforward. A reviewer that cannot
compile can only guess at a finding that needs one. ai-review scopes its
own model to `Read,Bash(gh pr diff:*),Bash(gh pr view:*)` and gives up
that capability. That acceptance covers code from inside the
organisation.

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.
required: false
type: string
default: 'Bash,Read'
Expand Down
Loading