Skip to content

ci: wire the seidroid review workflow - #251

Merged
bdchatham merged 6 commits into
mainfrom
feat/seidroid-review
Sep 10, 2026
Merged

ci: wire the seidroid review workflow#251
bdchatham merged 6 commits into
mainfrom
feat/seidroid-review

Conversation

@bdchatham

@bdchatham bdchatham commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

What this is

One new file, .github/workflows/seidroid.yml — a thin caller for
sei-protocol/uci/.github/workflows/seidroid-review.yml. Pure wiring: triggers, token
scopes, secrets, and four with: inputs. The review logic lives in uci and the reviewer
binary in sei-protocol/sei-internal-skills; nothing in this repository implements any
of it. No Go file changes.

Three jobs, mirroring the four repositories already wiring this reviewer
(platform, sei-k8s-controller, sei-internal-skills, sei-load):

job fires on mode
seidroid-review a non-closed pull_request, or @seidroid review in a top-level comment / inline review comment / review body review
seidroid-review-close @seidroid review close at the start of a top-level comment close
seidroid-review-reclaim pull_request: closed close

mode: close deletes the managed review session and reclaims its sandbox. It does not
close the pull request.

The inline-review-comment and review-body triggers follow platform specifically,
which added them first; sei-k8s-controller, sei-internal-skills and sei-load wire
issue_comment and pull_request only. They add places a person can type the command,
not a capability — a top-level comment already asks for a re-review of the current
tree.

Four deliberate divergences from this repository's conventions

Each is commented in the file so it does not read as an oversight.

  1. Raw sha pin, where uci-lint.yml, uci-release-check.yml,
    uci-release-publish.yml and uci-stale-check.yml all pin a uci tag
    (@v0.0.3, @v0.0.11). This is availability, not preference: the callee is on no
    uci tag and on no uci default branch — the workflow file has no history on uci's
    main, and uci's latest release does not contain it. There is no tag to name. All
    four existing callers pin this same sha, and the file is bumped across repositories
    by path, so the three occurrences here are kept identical.

  2. No concurrency: block, where all four uci callers in this repository set one
    with cancel-in-progress: true. A caller could express per-job groups here — this
    is a judgement that it should not, not a claim that it cannot. The callee already
    carries its own job-level concurrency, keyed on mode + repository + PR number with
    cancel-in-progress: true, so a newer review cancels an in-flight review while a
    teardown for the same PR sits in a separate group and survives; a caller group on top
    could only narrow that. And the group a caller would reach for first is the wrong
    one: a workflow-and-PR-number group with no mode in the key would couple the review
    job to the reclaim job, so a review arriving on a just-closed PR could cancel an
    in-flight reclaim, and mode: close is the path that reclaims a sandbox.

    Cancelling a review also frees nothing, and stops less than the word suggests.
    Cancellation unwinds the driver without deleting the session: the conversation stays,
    the sandbox stays allocated until something reclaims it, and the remote turn may
    keep running
    — the next request queues behind that turn rather than racing it. A
    caller-level cancel-in-progress would therefore buy latency, not reclamation, which
    makes the case for omitting it stronger rather than weaker.

    To be precise about deletion, since an earlier draft of this PR overstated it: close
    is the explicit end-of-work path, not the only code path that can delete a session.
    Driver v0.17.0 createOrAdopt (internal/omni/host.go) adopts a live or revivable
    session, but deletes and replaces one it finds that can no longer run a turn — repair
    of an already-dead session rather than a teardown a caller can schedule. Reviews do
    not routinely destroy sessions.

    Worth stating plainly, since it is easy to read more into it: the callee's grouping is
    not an exactly-once or session-lifecycle guarantee. A push and a comment can each
    admit a review of the same tree; overlapping runs cancel, sequential requests are not
    deduplicated; review and close sit in separate groups by design; and the comment path
    does not test whether the PR is already closed.

  3. permissions: {} at the top level with per-job grants, where this repository's
    other workflows grant concrete scopes at the top level. A reusable workflow may only
    downgrade what its caller granted, so the caller sets the ceiling. Sufficient
    inherited permissions would serve as well — the choice here is to grant the required
    scopes explicitly rather than depend on a configured default. Empty at the top means
    no job inherits one, and each job then states exactly the scopes the callee's two jobs
    declare between them (contents: read, pull-requests: write, checks: write,
    issues: write), so what this caller grants does not move with whichever
    GITHUB_TOKEN default the org or the repo has set — a setting we cannot read from
    here (403).

  4. Lowercase name: seidroid review, where this repository Title-Cases (Go Linter, Releaser, Stale Checker). seidroid review is the literal command a
    person types, and the callee sets run-name: UCI / seidroid review / #<n>;
    Title-Casing it would put this workflow's name at odds with its own trigger phrase
    and with every other caller.

Filename, relatedly: seidroid.yml, not uci-seidroid-review.yml. This
repository's uci- prefix denotes the uci boilerplate-CI family, where the caller
basename mirrors the callee basename (uci-lintgo-lint.yml, uci-stale-check
stale-check.yml) — it does not mean "calls sei-protocol/uci" generally
(sei-internal-skills holds both uci-release-publish.yml and seidroid.yml, both uci
callers). seidroid.yml is what all four existing callers use, and since the pin is
bumped fleet-wide by path, a repository-local spelling would be missed by the next
bump. The header comment's first line carries the provenance the prefix would have.

Inputs, and what is left unset

mode, approve-on-success: true, allowed-team: 'sei-protocol/sei-core', and
guidelines-file: CLAUDE.md. Every other callee input is left at its default for fleet
parity — deliberately, not by omission.

  • approve-on-success: true — on a clean conclusion the callee attempts an
    approving review. It is an attempt, not a guarantee, and the fallback is narrower than
    "falls back to a comment": the rung fires only where the API reports HTTP 422, in
    which case it attempts a COMMENT review. A refusal arriving any other way — a 403
    from an Actions approval policy, or a response carrying no status — gets no second
    attempt, and a post that does not succeed leaves a warning. Whether an approval that
    does land counts toward a required approval depends on branch policy and reviewer
    eligibility, neither of which this file decides. Revisit when that policy changes.

    Dated observation, as of this PR and deliberately kept out of the file's comments
    because it will age: rules/branches/main returns [], and PR feat: select storage performance by its (IOPS, throughput) pair #249 merged with zero
    reviews (pulls/249/reviews[]), so no approving review is required on main
    today. The classic branch-protection API is 403 to the token used here, so this is
    strong indirect evidence rather than a direct read.

  • guidelines-file: CLAUDE.md — unset reads REVIEW.md, which this repository does
    not keep (there is no file with "review" in its name anywhere in the tree), so the
    callee would fetch a 404 and fall back to a generic Go checklist. CLAUDE.md lines
    19–44 are an enforceable ## Code Standards section, and lines 42–44 carry a
    cross-repo invariant a generic checklist cannot know: the sidecar HTTP contract lives
    in sei-k8s-controller/sidecarapi, and a change there means bumping the go.mod
    dependency after it lands. This follows the considered precedent rather than the
    majority one: of the four existing callers only sei-k8s-controller wires
    guidelines-file. platform and sei-load keep no CLAUDE.md at all, so they have
    nothing to point at; sei-internal-skills has one and still passes nothing.
    Restricting to callers that could wire it, it is 1 of 2, and this PR makes it 2 of 3.

Fork safety, and the one fork caveat

An automatic pull_request run from a fork is denied by the callee's guard
(origin=forkdeny "…fork-originated; not reviewing it"), so no review session and
no model turn start for fork code. A non-member issue_comment is filtered by author
association. This repository being public is therefore not a new exposure.

The caveat, which is commented on the reclaim job: GitHub withholds secrets from a
pull_request run whose head is a fork, so the automatic reclaim cannot authenticate.

It is narrower than it first reads, and that is worth stating. At this pin there is
no supported path that creates a session for a fork-originated PR at all: both review
paths require equal head/base repo ids and deny an unreadable origin; an outside
collaborator pushing to a branch inside this repository yields a same-repo PR, since
their identity does not make the head a fork; retargeting the base, toggling draft and
synchronizing the head all leave the head repository unchanged; and mode: close
bypasses the origin checks but only tears down — driver v0.17.0's Close
(internal/omni/host.go) searches for sessions carrying the run key and deletes what it
finds, creating nothing, and its close path returns before review execution
(cmd/sei-agent-driver/main.go). So the caveat covers a pre-existing session created
outside these guarded review paths
— not a routine leak this wiring causes.

Where such a session does exist, the teardown has to come from an author the guard
admits.
The guard job's if: requires author_association in OWNER, MEMBER or
COLLABORATOR; the team-membership check exempts a teardown, but this association check
is in the guard's own if: and is not mode-aware. Note that an outside collaborator
holds COLLABORATOR and so qualifies — the excluded case is an author with no admitted
association, not "every external contributor". If your association is not one of those,
ask an admitted collaborator or a maintainer to post it before merging.

The four existing callers all carry "On a fork pull request, comment @seidroid review
close before merging" verbatim (platform:90, sei-k8s-controller:63,
sei-internal-skills:62, sei-load:61). That line does not name an audience, so a
maintainer reading it can follow it correctly; what it omits is the authorization
requirement, which is what makes it misleading to a contributor who cannot act on it.
Worth a fleet-wide comment fix, as an omission rather than a wrong instruction.

Known limitation — inherited, not introduced here

The caller and the callee grade the same comment body by different rules, and the callee
never checks the line it parsed against the mode the caller routed. When they disagree,
the caller picks the job while the callee picks whether to run at all. They diverge on
three axes, not one:

axis caller (if: expressions) callee (guard's grep -E)
match unit substring anywhere in the body a whole line reading @seidroid review [close], nothing else on it
case insensitivecontains and startsWith are documented "not case sensitive" sensitive — no -i; the trigger-phrase input states "@Seidroid review starts no review"
boundary string prefixstartsWith has no notion of where a token ends token — close has to be a whole word

Only the first axis was described here until seidroid's review of this pull request
flagged the other two; both widen the destructive surface, and one of them defeated the
guidance this section used to give. Credit at the end of the section.

The case that matters, because it is destructive:

@seidroid review close is an example, not a request.

@seidroid review

The body begins with the teardown phrase, so startsWith selects the close job (and
the review job's !contains excludes itself). The callee then scans for a whole
matching line: line 1 has trailing words and does not match, but line 3 does, so it
proceeds — with mode: close. The session is deleted. The author asked for a review
and lost their session.

Lesser variants: please @seidroid review is admitted by the caller and rejected by the
callee (no whole line), so it silently does nothing; @seidroid review followed by a
blockquoted > @seidroid review close skips both jobs; a standalone @seidroid review
inside a fenced code block still starts a review.

Two further routes to that same destructive path, neither of which requires typing
the teardown command as written.

Case. The caller's startsWith ignores case; the callee's grep does not:

@Seidroid review close — for example, do not do this

@seidroid review

startsWith matches the capitalised first line, so the close job is selected and the
review job excludes itself. The callee then skips line 1 (wrong case for its grep) and
matches line 3, and proceeds under mode: close. Session deleted. The mirror direction is
harmless: a lone @SEIDROID REVIEW selects the review job and then parses to nothing, so
it silently does nothing.

Word boundary. startsWith tests a prefix, not a token, so close need not be a whole
word:

@seidroid review closely at the retry logic — the backoff looks wrong

@seidroid review

@seidroid review close is a prefix of @seidroid review closely, so the close job is
selected. The callee rejects line 1 (trailing words) and matches line 3. The session is
deleted, and the author never typed the teardown command anywhere in the body.

@seidroid review close-not-now goes the same way. This is the least intuitive of the
three axes and the reason the guidance below needed rewriting.

This is a property of the shared caller/callee pair and this PR neither introduces nor
worsens it. The parser/mode mismatch and the destructive reproducer are shared by every
caller, but close-job routing is not identical across them: only platform anchors
with startsWith (:73); sei-k8s-controller (:46), sei-internal-skills (:45) and
sei-load (:44) all use contains. That changes the lesser variants there — the
blockquoted > @seidroid review close does not skip both jobs in those three, because
their close job is selected by the substring and can then act on a separate valid review
line. This file follows platform. It is not fixable in a caller:
GitHub expressions have no regex and no newline literal, so any caller-side grammar we
invented would produce a different set of wrong answers rather than fewer, and divergent
if: logic here would fight the next fleet-wide pin bump. The fix belongs in the callee,
which is the only reader that can compare the line it parsed to modeplatform's
copy carries a PLT-1182 reference saying exactly that, so upstream already knows.

Practical guidance until then. This replaces an earlier version of this paragraph
that said only "don't begin a comment with the teardown phrase unless you mean it" —
which is insufficient, because the author of @seidroid review closely at the retry logic has typed no such phrase and can still lose their session:

  • Put the command on a line of its own, with nothing else on that line. That is what
    the callee actually matches, and it is the only form that reliably means what it says.
  • Do not open a comment with @seidroid review followed by anything starting clos,
    in any casing, unless you intend a teardown. close, Close, closely and
    close-not-now all select the teardown job.
  • To discuss the teardown command rather than run it, keep it off the first line. A
    mention later in the body cannot select the close job in this caller.
  • When in doubt, do not lead a comment with @seidroid at all unless the command is the
    entire line.

Provenance: the case and word-boundary axes were found by seidroid[bot] reviewing this
pull request — an inline suggestion on this very file (comment 3983292798, on
.github/workflows/seidroid.yml). The tool being installed found a real gap in the
documentation of its own wiring, on its own installation PR, and asked for no change to
the expressions. Its verdict on the change was review found nothing blocking
(APPROVED, 2026-09-10T20:39:40Z).

PREREQUISITE — now satisfied, observed on this pull request's own run

The org secrets OMNIGENT_MACHINE_CLIENT_SECRET, PLATFORM_CODE_AGENT_APP_ID and
PLATFORM_CODE_AGENT_APP_PK must include sei-protocol/seictl in their repository
access.
This repository referenced no secrets in any workflow before this change,
so it was not safe to assume it appeared in a "selected repositories" list, and every
secrets API returns 403 to the token used to prepare this change. It was written up here
as unverified.

It is now verified empirically, because this workflow ran on the pull request that
adds it.
The pull_request run exercised the added workflow through the PR merge
ref, so opening this PR tested the real wiring rather than a fake. Run
34523934052, job Guard, reports by step name:

Guard step Result
4. Mint an identity to ask about the team and the labels success
5. Admit the request success
6. Require the machine-client secret success
7. Report a half-configured reviewer identity success

Step 6 is the callee's explicit presence check, so its success is direct evidence that
OMNIGENT_MACHINE_CLIENT_SECRET reaches this repository. Step 4 running rather than
being skipped, plus the Review job's Mint the reviewing identity step succeeding,
indicates the App credentials are present too. The close and reclaim jobs skipped
correctly on a pull_request event, which is the three-job routing behaving as intended
against a real payload.

One limit on that evidence, stated rather than glossed: it demonstrates the secrets are
reachable from this repository now. It does not show which mechanism grants them
(org-wide, selected-repositories, or repository-level), so a reviewer who wants that
recorded should still check the settings. Nothing about the merge is blocked on it.

For reference, had it NOT been satisfied, missing credentials would cause the following
failures or degraded behaviour. The two credentials fail in different places and to different degrees, so both
are worth stating separately:

Missing OMNIGENT_MACHINE_CLIENT_SECRET — the callee's guard has an explicit
presence check that exits 1 with OMNIGENT_MACHINE_CLIENT_SECRET is not set on the calling repository, before the driver attempts to mint the machine-client bearer. (Not
before any token: where the App credentials are configured the guard mints an App token
earlier, to read team membership and labels.) That check is gated on the request
already being admitted, so it fires on reviews that would otherwise have run and not on
requests already denied (fork-originated, draft, skip-labelled, unauthorised commenter)
— those refuse for their own reasons and stay green. The pull_request: closed reclaim
skips the guard entirely, so it has no presence check to fail and would instead fail
later, in the driver, at minting. So "fails on every pull request" is too broad: it fails
every pull request that reaches a review, which on this repository is expected to be most
of them.

Missing PLATFORM_CODE_AGENT_APP_ID / _APP_PK — not fatal, and a different
failure. The review runs but posts as github-actions[bot] rather than the bot, and
every @seidroid review comment is refused, because team membership cannot be read
without the App identity. Automatic pull_request reviews and authorised
@seidroid review close still work. If only one of the two halves is set, the callee
warns and carries on under the workflow's own identity.

No action is required before merge. The paragraphs above are kept because they document
what each credential does and how each one fails, which is worth having written down the
first time this repository depends on any secret at all.

How this was verified

  • actionlint v1.7.7 (release binary, downloaded — this host has no Go toolchain),
    run over the whole workflows directory. Clean, exit 0, all six files:
    $ actionlint -no-color .github/workflows/*.yml
    $ echo $?
    0
    
    Verified the gate is actually live with a negative control — a copy of this file with
    a bogus activity type, which it caught:
    zz-negctl.yml:39:24: invalid activity type "bogus_type" for "pull_request_review" Webhook event. available types are "dismissed", "edited", "submitted" [events]
    
    Honest limit of that gate: the same negative control also fabricated
    github.event.no_such_field, and actionlint did not flag it — github.event is a
    loosely typed payload, so the github.event.* paths in the if: expressions are not
    machine-validated. The two review events and the github.event.review.body fallback
    match platform's live production usage specifically — it is the only existing
    caller that wires them.
  • YAML parse backstop via python3 -c 'import yaml; yaml.safe_load(...)', then
    checked the parsed keys rather than trusting the parse. To be exact about what came
    back: the parsed top-level keys are 'name', boolean True, 'permissions' and
    'jobs' — PyYAML (YAML 1.1) coerces the bare on key to a boolean, so there is no
    'on' string key at all. That is a parser artifact, not a defect: every existing
    workflow in this repository and all four sibling callers parse identically, and
    GitHub's own parser reads it as a string, so on: was deliberately left unquoted
    rather than turned into a divergence. Under that boolean key the four events and their
    types are as intended; permissions is an empty dict; jobs holds
    seidroid-review, seidroid-review-close, seidroid-review-reclaim; there is no
    concurrency key.
  • Every claim about callee behaviour in the file's comments was read out of the callee
    at the pinned sha, or the driver at its pinned version
    — the job-level concurrency
    group, the fork denial, the whole-line command grammar, the 422 fallback rung, the
    session adopt/replace logic, and the effective REVIEW.md default.
  • One correction to that method, which is worth stating because it caught a real error:
    an earlier draft described cancellation as stopping the session. That came from the
    callee's own comment, not from code, and the driver contradicts it — see item 4
    below. Reading a callee's prose is not the same as reading its behaviour, and the
    cancellation wording now follows the driver.

No Go file changed, so the Go gates do not apply and were not run — this host has
neither go nor make. This repository's own Go Linter workflow will run on this PR
regardless.

No pre-existing gate in this repository validates workflow YAML. uci-lint.yml
calls uci's go-lint.yml, which lints Go, not Actions. The actionlint run above was
done by hand and is not enforced by CI, so please do not read a green CI here as
validation of this file.

Six things worth fixing upstream, none carried across

  1. platform's approve-on-success comment says its main "carries no branch
    protection today, so the approval satisfies no required-review rule". False now
    platform ruleset 11598654 sets required_approving_review_count: 1. The comment
    here is written for this repository, and phrased so it stays true when policy moves.
  2. platform's close-job comment refers to the quoted teardown command as something
    "which the header above tells a fork author to do". The header says nothing about
    forks, and the instruction is on the reclaim job below. Corrected here.
  3. All four callers' fork remedy omits the authorization requirement, so a
    contributor without an admitted association can follow it and have nothing happen —
    see the fork section above.
  4. The pinned callee's own cancellation comment is wrong — and this is the first of
    these in the callee rather than in a caller
    , so every repository wiring it inherits
    the error. It says the driver "traps cancellation and STOPS its session, keeping the
    conversation". At driver v0.17.0: the signal handler cancels only the local context
    (cmd/sei-agent-driver/main.go); a turn is explicitly described as still running
    after a cancelled context or an expired deadline, with the next invocation's prompt
    queueing behind it (internal/driver/driver.go); and the package doc says a run
    tears nothing down because stopping a session ends the agent process and the runner
    but never the sandbox, the runner's idle timeout ending that process unasked
    (internal/driver/doc.go). This file's earlier wording came straight from that
    comment
    , which is the useful half of the finding: a callee's prose is load-bearing
    for its callers, and this line of it is stale.
  5. All four callers present the fork caveat as though the automatic reclaim were
    leaking sandboxes in the ordinary course. At this pin no supported path creates a
    session for a fork-originated PR, so the caveat only ever covers a pre-existing or
    out-of-band session. Worth rewording upstream so the caveat is not read as a routine
    leak — and worth checking against a newer driver, since this rests on v0.17.0.
  6. The driver contradicts itself about session deletion, in two files.
    cmd/sei-agent-driver/main.go:206 says "review deletes no session by design, and
    only --close does", while internal/driver/driver.go:80-81 says Close ends a
    session "-- though opening will delete a session it finds unable to run a turn at
    all", repeated at driver.go:150-151. The second is the one that matches the code
    (internal/omni/host.go adopts when live or revivable and deletes only after that
    test fails). Listed with narrower scope than the others: it is in the driver rather
    than the callee, so a caller does not inherit it directly. No claim is made here
    about how the same error reached this file's earlier draft — matching prose does not
    establish provenance.

All six are comment-only and out of scope for this PR: four are in other repositories,
one is in the callee, and one is in the driver.

The caller/callee grammar mismatch is the one upstream problem that is not
comment-only. It is documented separately, under Known limitation above, rather
than as a seventh entry in this list. platform tracks it as PLT-1182. What this PR
adds to it: the mismatch has three axes, not the one the fleet's comments describe
— match unit, case, and token boundary — and the two beyond match unit are the ones that
let a body reach the destructive path without the teardown command ever being typed.
Whoever picks up PLT-1182 should address all three in the callee, since a caller
cannot. Note that comparing the parsed command line to mode is necessary but not
sufficient: it prevents executing the wrong operation, which is the destructive half,
but it does not align the two grammars. A review request followed by a blockquoted
teardown is never dispatched, while a lone uppercase request is dispatched and then
rejected by the callee — two different failure points. Both remain silent no-ops, and
fixing them needs the accepted grammar aligned as well. Both additional
axes came from seidroid[bot]'s own review of this pull request.

omnigent and others added 5 commits September 10, 2026 18:41
A thin caller for sei-protocol/uci's seidroid-review.yml. Review logic and the
reviewer binary live upstream; this file only wires triggers, scopes and secrets.

Four deliberate divergences from this repository's other workflows, each commented
in the file: the raw sha pin (the callee is on no uci tag and on no uci default
branch), the absence of a `concurrency:` block (the callee serialises per mode, and
a caller-level group would let a review cancel a sandbox reclaim), the empty
top-level `permissions:` with per-job grants, and the lowercase `name:`.

Co-authored-by: omnigent <noreply@omnigent.ai>

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The permissions comment said each job states "the four the callee's own jobs
declare". The callee's guard declares pull-requests: read and issues: read; only
its review job declares all four. Reword to the union, and give the actual reason
the block is empty: a reusable workflow may only downgrade what its caller
granted, and the default token grants contents, packages and metadata only.

Co-authored-by: omnigent <noreply@omnigent.ai>

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment-only. The non-comment content of the file is byte-identical: the three
`if:` expressions, `on:`, `permissions:`, `uses:`, `secrets:` and `with:` are
unchanged.

- The review job claimed a request "still reads mid-sentence". False: the callee
  requires a whole line reading `@seidroid review [close]`, so `please @seidroid
  review` is admitted here and rejected there. Replaced with the actual mismatch --
  caller matches substrings, callee matches lines and never checks the parsed line
  against `mode` -- and a pointer to the PR description, which carries the case
  where that deletes a session.
- The close job claimed anchoring stops a quoted teardown from destroying a session.
  It only stops the phrase occurring LATER in a body; a body that begins with it
  still routes there whatever follows. Says that now.
- `approve-on-success` claimed a clean review "publishes a real approving review,
  not a comment". The callee attempts APPROVE and falls back to COMMENT when the
  API refuses the position, or records none at all. It also asserted `main`'s
  current policy, which dates; that evidence moved to the PR description.
- The permissions comment asserted the default token grants contents, packages and
  metadata only. That is the restricted default; the effective one is an Actions
  setting we cannot read. Reworded to be independent of it.
- The no-concurrency rationale said the callee's granularity is one "a caller cannot
  express". A caller can set job-level groups. The decision stands on judgement, not
  impossibility, and the argument now says so.
- The trigger comment claimed a re-review otherwise needs a manufactured empty
  commit. A top-level issue_comment already works; the extra events add places to
  type the command, not capability.
- The fork remedy told the fork author to post `@seidroid review close`. The guard
  admits a teardown only from an OWNER, MEMBER or COLLABORATOR, so an external
  contributor cannot. Now addressed to a maintainer, and the sandbox consequence is
  conditional on a session existing.

Also records what the callee's concurrency is not: no exactly-once, no session
lifecycle, no closed-PR check on the comment path.

Co-authored-by: omnigent <noreply@omnigent.ai>

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment-only again; non-comment content byte-identical (same md5 as the two
preceding commits).

The concurrency note said cancellation stops a session and "only a close deletes
it". Driver v0.17.0 createOrAdopt (internal/omni/host.go) deletes and replaces an
existing session it finds that can no longer run a turn, so opening a review can
delete one. Close is now described as the explicit end-of-work path that reclaims a
sandbox, with the repair path named and marked as repair rather than teardown.

Five residual inaccuracies from the previous pass, all comment prose:

- The permissions note still said a caller "has to hand the callee its scopes
  explicitly". Sufficient inherited permissions serve too; the explicit grant is a
  choice not to depend on a configured default.
- The no-concurrency argument justified itself against "workflow + ref, as this
  repository's other callers key". Those four callers use three different shapes,
  none uniformly that, and workflow+ref is not PR identity anyway (comments carry
  the default-branch ref, PR events a merge ref, closed events the target branch).
  Argues from a workflow-and-PR group without mode instead, which is the shape that
  would actually couple review to reclaim.
- approve-on-success said the callee falls back to a comment "where the API refuses
  that position". The rung fires only on a reported 422; a 403 approval-policy
  refusal gets no fallback, and the fallback attempt can itself fail.
- The close job claimed anchoring the review job too would trade a misroute for a
  silent no-op. In the documented case the review job already excludes itself, so
  re-anchoring would have prevented nothing. Now states fleet parity as the reason.
- The fork caveat implied a routine leak. No routine path creates a session for a
  fork-originated PR -- the callee refuses a review on one from every trigger -- so
  the caveat covers a pre-existing or out-of-band session. Also corrected the
  authorization: the gate is on admitted association, and an outside COLLABORATOR
  qualifies, so it is not "every external contributor" who is excluded.

Co-authored-by: omnigent <noreply@omnigent.ai>

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment-only; non-comment content still md5 c64a681798c7, unchanged across all
five commits.

The concurrency note said the driver "traps cancellation and stops its session,
keeping the conversation". That came from the callee's own comment, and the callee
is wrong at this pin. Against driver v0.17.0: the signal handler cancels only the
local context (cmd/sei-agent-driver/main.go), a turn is explicitly described as
still running after a cancelled context or expired deadline with the next prompt
queueing behind it (internal/driver/driver.go), and the package doc says a run
tears nothing down because stopping a session ends the agent process and runner
but never the sandbox, the runner's idle timeout ending that process unasked
(internal/driver/doc.go).

So cancellation unwinds the driver without deleting the session: the conversation
stays, the sandbox stays allocated until something reclaims it, and the remote turn
may keep running. This strengthens the reason for omitting a caller concurrency
block rather than weakening it -- a caller-level cancel-in-progress would buy
latency, not reclamation -- and the bullet now says so.

Also drops "predates the head becoming a fork" from the fork caveat. It invented a
transition nobody has demonstrated, and contradicted the same paragraph's correct
statement that retargeting, draft changes and synchronization leave the head
repository unchanged. Now reads as a pre-existing session created outside these
guarded review paths.

Co-authored-by: omnigent <noreply@omnigent.ai>

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@cursor

cursor Bot commented Sep 10, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Introduces automated PR reviews and optional approvals using org secrets and write scopes on pull requests/issues, but changes are workflow-only and mirror existing fleet wiring.

Overview
Adds .github/workflows/seidroid.yml, a thin GitHub Actions caller that hooks this repo into the shared @seidroid review automation via sei-protocol/uci’s reusable seidroid-review.yml (pinned to commit d895e04).

Review runs on PR open/sync/reopen and when @seidroid review appears in issue, inline review, or review-body comments (excluding @seidroid review close). Close tears down the managed session when a top-level comment starts with that phrase. Reclaim runs mode: close when the PR is closed. Repo-specific inputs include approve-on-success: true, guidelines-file: CLAUDE.md, and allowed-team: sei-protocol/sei-core, plus org secrets mapped from PLATFORM_CODE_AGENT_APP_* and OMNIGENT_MACHINE_CLIENT_SECRET.

The file intentionally differs from other uci callers here: no top-level concurrency (the callee owns grouping), permissions: {} with per-job scopes, lowercase workflow name, and a raw SHA pin because the callee isn’t on a uci tag yet.

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

@seidroid seidroid 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.

Findings on the changed lines. The verdict and the summary are in this tool's comment on this pull request.

if: >-
github.event_name == 'issue_comment' &&
github.event.issue.pull_request != null &&
startsWith(github.event.comment.body, '@seidroid review close')

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 — Unchanged since the last review, and still worth the comment fix. Verified again against the callee at the pinned sha: the guard's parse step greps for a whole line matching ^\s*@?seidroid\s+review(\s+close)?\s*$ and writes should_run=true without ever comparing the line it matched to inputs.mode, so this expression alone decides review-vs-close. The destructive case the description records reproduces exactly as written, and the review job's !contains on line 99 is what makes the misroute exclusive rather than additive. That much the comment above already says.

It is short on two further axes, and both widen the trigger surface beyond what a reader of the comment or the description would expect.

CASE. GitHub's startsWith and contains are documented as case-insensitive; the callee's grep -E is case-sensitive, and the trigger-phrase input says so explicitly (@Seidroid review starts no review). A body opening @Seidroid review close ... therefore routes to this job, and if it also carries a properly-cased @seidroid review line the callee proceeds under mode: close. The mirror case is benign: a lone @SEIDROID REVIEW selects the review job and parses to nothing.

WORD BOUNDARY. startsWith tests a string prefix, not a command token, so @seidroid review closely at the retry logic and @seidroid review close-not-now both select this job. Alone each is a silent no-op — the whole-line grep rejects the line — but with a later bare @seidroid review line each is the destructive path, and the author has typed nothing resembling the teardown command anywhere in the body. The description's guidance ("don't begin a comment with the teardown phrase unless you mean it") does not prepare a reader for that: the phrase was never typed.

Agreed the fix belongs in the callee and that inventing caller-side grammar here would fight the next fleet-wide pin bump; no change to the expression is being asked for. What is worth changing is the comment at line 82, which presents the divergence as substring-vs-whole-line only. Naming all three axes would let the next person weighing a caller-side fix see the full shape of what they would be fixing.

@seidroid seidroid 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.

review found nothing blocking.

@seidroid

seidroid Bot commented Sep 10, 2026

Copy link
Copy Markdown

1. Blocking

None.

2. Non-blocking

Nothing material has changed since my last review. The head is still 907f938, the merge commit is still cb24f538, the diff is still 181 lines against the same blob (e4be5e6), and a full re-read confirms it byte-for-byte identical. The base branch's CLAUDE.md is also unchanged, and the callee is pinned by sha so it cannot have moved underneath the file. No finding I raised is addressed, and none has been made stale.

Every note from my first review stands unchanged, so I will not restate them at length — they are carried in full in the JSON block below:

  • the close-job routing hazard at seidroid.yml:137 (inline);
  • scouts unset on all three jobs, safe only because they share one default;
  • a fork-headed pull_request: closed leaving a failed run, with no guard-side presence check to make it legible;
  • re-review-on-push: false against synchronize plus approve-on-success: true;
  • allowed-team omitted on the reclaim job alone;
  • the unverified org-secret prerequisite, still 403 to this token and still needing an org admin before merge.

One thing is worth adding to the routing finding rather than opening beside it. startsWith has no word boundary. @seidroid review closely at the retry logic and @seidroid review close-not-now both satisfy the close job's guard, because the test is a prefix test on the string '@seidroid review close' and not on a command token. On its own that is a silent no-op — the callee's whole-line grep rejects the line and the run does nothing — but combined with a later bare @seidroid review line it is the destructive path, and the author has typed nothing that resembles the teardown command anywhere in the body. That is a materially wider footgun than the description's guidance ("don't begin a comment with the teardown phrase unless you mean it") prepares a reader for. Together with the case-insensitivity gap I raised last time, the comment at seidroid.yml:82 characterises the caller/callee divergence along one axis of three.

3. Summary

The pull request has not moved: same head, same merge commit, same 181-line diff, same blob, and the base standards are unchanged, so this is a re-review of identical material and every earlier finding carries forward untouched. One scout ran: codex raised the close-job routing hazard again, this time with a close-not-now example; I verified it, kept it, re-anchored it to line 137 (its :145 pointer lands on checks: in the close job's permissions: block, not the guard), and folded the word-boundary dimension it surfaces into the existing inline comment alongside the case-insensitivity dimension, so the author is left with one thread rather than two. It contributed nothing else, and it argued against none of my findings. The conclusion is unchanged: the routing hazard is inherited from four sibling callers, tracked upstream as PLT-1182, bounded to teardown of an ephemeral review session, and not fixable in a caller without diverging from the fleet — so it does not block a wiring change that neither introduces nor worsens it. Approving again, with the org-secret prerequisite still the one item that needs a human before merge.

seidroid review · decision approve · session eed15a294ea04e4eab0935c745f1cdf1 · turn resp_claude_5083d7480d43fb80a3aa5e69ff75c784 · item 3ecc36fb812b5f1fb05849246558f271

Findings: 0 blocking | 7 non-blocking | 1 posted inline

Comment-only; non-comment content still md5 c64a681798c7, unchanged across all
six commits.

The review job's comment described the divergence as substring-vs-whole-line. It
has three axes, and the two that were missing both widen the destructive surface:

- CASE. GitHub documents contains() and startsWith() as "not case sensitive". The
  callee's parse is `grep -m1 -E` with no -i, and its trigger-phrase input says so
  outright: "Matched case-sensitively. `@Seidroid review` starts no review." So a
  body opening `@Seidroid review close ...` selects the close job here while the
  callee skips that line, and a properly-cased `@seidroid review` line later in the
  body then runs under mode: close.
- WORD BOUNDARY. startsWith tests a string prefix, not a command token, so
  `@seidroid review closely at the retry logic` selects the close job. The author
  has typed nothing resembling the teardown command.

No expression changed: a caller cannot grade this reliably, and the callee is the
only reader that can compare a parsed line to mode. The comment now names all
three so the next person weighing a caller-side fix sees the whole shape of it.

Found by seidroid[bot] reviewing the pull request that adds this file -- an inline
suggestion on this file, which also approved the change. The PR description carries
the reproducers and the revised guidance; the previous guidance was insufficient,
since it told readers not to open a comment with the teardown phrase and the
word-boundary case never types that phrase at all.

Co-authored-by: omnigent <noreply@omnigent.ai>

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@bdchatham
bdchatham merged commit 3e0b8be into main Sep 10, 2026
7 checks passed
@bdchatham
bdchatham deleted the feat/seidroid-review branch September 10, 2026 21:31
@bdchatham
bdchatham restored the feat/seidroid-review branch September 10, 2026 21:34
@bdchatham

Copy link
Copy Markdown
Contributor Author

@seidroid review close

Reclaiming the sandbox left behind by the merge. The pull_request: closed reclaim job
ran and failed at teardown (run 34532635137, exit 8): the scout session was deleted, but
the review session's close could not reach the server —
POST https://seigent.dev.platform.sei.io/oauth/token: context deadline exceeded
against a 120s budget — so it exited with teardown_ok: false and
close failed (exit 8) — the session and its sandbox are still running.

Same transport unreachability that made the first review attempt on this PR expire its
20-minute run deadline before a reply arrived; the retry then succeeded. Posting the
documented remedy rather than leaving the sandbox held.

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