fix(seidroid-review): raise the driver floor to v0.18.0 - #106
Closed
bdchatham wants to merge 30 commits into
Closed
Conversation
A feature branch on purpose. Nothing here merges to main and no release contains it, so a repository opts in by pointing its caller at this ref and opts out by removing that file. No uci workflow triggers on push, tag or release, so a push to this branch fires nothing. The file is the one from sei-protocol/sei-internal-skills, adapted in seven places. Six are comments and one is an input description; no executable line differs, and actionlint reports the same three pre-existing shellcheck style notes as the source. The description mattered most. It said driver-version takes "a commit sha of this repository", which was true where the file lived and is false here: the driver is a nested Go module in sei-internal-skills, and moving this workflow did not move it. A caller who read that here would pass a uci sha and `go install` would fail on an unknown revision. The driver is installed by absolute module path, so nothing else moved with the file. It checks out no repository, so no asset had to come across and no fork code runs on the runner. This is a second reviewer, not a replacement. It publishes its check run as `review` rather than `AI Review` because both systems run during the transition and both post as seidroid[bot], and it triggers only on an explicit `@seidroid review` comment, so it cannot report on a pull request nobody asked about. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The workflow was manual-only: the guard admitted `issue_comment` alone, and the review job's `pull_request` branch required `mode: close`. A caller passing `mode: review` on a `pull_request` event matched neither branch, so the job skipped and the automatic path stayed with `ai-review.yml`. Admit `pull_request` + `mode: review` in both conditions. The guard's parse step reports the same outputs for that event as the command grammar does, so the review job reads one shape and needs no second code path. Two gates come with it, because an automatic run spends model quota on every push rather than when a person asks: - Refuse a draft. Read from the event payload, so it needs no identity and cannot fail open. The comment path still reviews a draft on request. - Apply `allowed-team` to the comment path only. It gates who may COMMAND a review; applying it to an automatic run would silently stop reviewing every pull request opened from outside the team. No author-association check on the automatic path, matching the workflow this replaces: the trigger is the push, and GitHub withholds this workflow's secrets from a fork pull request, so such a run fails the machine-client check instead of reviewing unauthorised code. The close-on-pull_request path is unchanged and still skips the guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ate the finding counts (#71) Two changes to how a review reaches the pull request, landed together because they are the same concern. FAIL SOFT ON PUBLISH. The sticky upsert was the only publish step without continue-on-error, and the last step in the job, so a failed gh api under set -euo pipefail discarded a review that had already cost model spend and held a sandbox. It now fails soft -- but silence would be worse than a red job, so two signals replace the exit code: an ::error:: annotation printing the unposted body to the log, and a check run named review with conclusion failure, which supersedes the green on that sha and clears on a re-run. $NOTE is bounded against GitHub's comment cap, cut on whole lines, with the count shown in the comment and the whole note in the log. STATE THE FINDING COUNTS. The place step already computed on_line, on_file and unplaced per finding and echoed them to the log; it now exports them. The verdict step composes the Findings line from those plus the integer counts the driver writes to check.json as of sei-agent-driver/v0.11.0, and appends it after the marker line -- never before it, which would orphan every open pull request's sticky comment. The counts do not reproduce ai-review's. Its blocking total includes a pre-existing blocker beside a gate that excludes one on purpose, and its non-blocking total drops a suggestion whose line fell outside the diff. A caller on an older driver gets a shorter, still-true line rather than a broken one: jq reads absent keys as null and the step logs the skew. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
## What
Reacts 👀 to the triggering comment as the **first** step of the `review`
job.
## Why
A review takes minutes to produce its first visible output — the Go
toolchain, the driver install, and the session start all run before
anything
appears on the pull request. Until then nothing on the PR distinguishes
"the
trigger was seen" from "the trigger was dropped", and the person who
asked
has to open the Actions tab to find out which.
This is not hypothetical. During a credential outage, runs completed
`success` and posted nothing, and the only way to tell a working review
from
a broken one was to read the workflow log. An acknowledgement would have
separated the two immediately.
It is the first step deliberately. An acknowledgement that arrives after
the
verdict is not one.
## Scope
- **Comment path only.** An automatic `pull_request` review has no
comment to
react to, so the guard leaves `comment_id` empty and the step is
skipped.
- **`continue-on-error`.** An acknowledgement is a courtesy. Failing the
review because a reaction did not post would trade the whole job for the
signal that the job started. A failure emits a `::warning::` instead.
- **Idempotent.** Reactions are unique per (user, content), so
re-running a
review on the same comment returns the existing reaction rather than
adding
a second one. A retry needs no cleanup.
## The permission
The job gains `issues: write`. A reaction on a *pull request* comment
goes to
the `/repos/{repo}/issues/comments/{id}/reactions` endpoint, which
`pull-requests: write` does not cover — the likely reason this was never
wired up.
## Verification
`actionlint` is clean on the new step; the four findings it reports are
pre-existing, at lines 326/1001/1177.
Behaviour needs a live trigger to confirm, since it depends on the
token's
effective permissions in the calling repository. The consumer-side pin
bump
is [sei-load#97](sei-protocol/sei-load#97).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
## What Replaces the sticky upsert with a plain POST. Each review posts a new verdict comment; earlier ones stay. ```diff -if id="$(gh api ".../comments" --jq "map(select(.body | startswith(\"$MARKER\"))) | .[0].id // empty")"; then - if [ -n "$id" ]; then - gh api -X PATCH "repos/$REPO/issues/comments/$id" -f body="$body" - else - gh api -X POST "repos/$REPO/issues/$PR/comments" -f body="$body" - fi -fi +if gh api -X POST "repos/$REPO/issues/$PR/comments" -f body="$body" >/dev/null; then + posted=true +fi ``` ## Why Editing a comment in place leaves it at its **original** position in the thread and notifies nobody. So a re-review did not move, did not notify, and silently destroyed the previous verdict's text — three properties that together make a verdict nearly indistinguishable from no verdict. This is not theoretical. Diagnosing a broken reviewer today, on sei-load#96 and #90: | | your trigger | verdict body written | rendered at | |---|---|---|---| | #96 | `23:18:45` | `23:22:49` | **`20:53`** | | #90 | `23:18:46` | `23:24:06` | **`21:19`** | Both verdicts were written 4–5 minutes after the trigger and rendered eighteen hours up the page. Runs completed green, the bottom of the thread showed nothing new, and the conclusion "seidroid is still broken" was wrong — the review had worked and its output was invisible. ## Why earlier verdicts stay They are the record of what the review said *before* the author's fixes, which is what a reader compares against. A long pull request accumulates a few; that is the accepted cost, and it is the behaviour of the `ai-review` tooling this replaced. ## A hazard this removes `<!-- seidroid-review -->` is also the marker on the unplaced-findings note posted by the preceding job (line 1099). The old lookup took `.[0]` — the **oldest** marker comment — so on a pull request where that note came first, the verdict would have overwritten the note instead of the previous verdict. ## Verification `actionlint` reports the same 5 pre-existing findings as `main` (lines 326/1001/1177); none from this change. YAML parses; the `posted` flag and its unposted-verdict fallback path are untouched. The consumer pin bump is [sei-load#97](sei-protocol/sei-load#97). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… them (#74) ## The bug `check.json` carries the counts nested under `counts`, beside a top-level `conclusion`. The step read the three counts at the **root**. ```console $ jq -r '[(.blocking // "?"), (.non_blocking // "?"), (.pre_existing // "?"), (.conclusion // "?")] | @TSV' check.json ? ? ? success $ jq -r '[(.counts.blocking // "?"), (.counts.non_blocking // "?"), (.counts.pre_existing // "?"), (.conclusion // "?")] | @TSV' check.json 0 2 1 success ``` The real shape, rendered by running `BuildCheckRun` on a verdict with two non-blockers and one pre-existing suggestion: ```json {"conclusion":"success","title":"2 findings, 1 pre-existing issue","summary":"…", "counts":{"blocking":0,"non_blocking":2,"placeable":0,"pre_existing":1}} ``` ## Three consequences, all silent 1. **The findings line never printed a number.** `is_count "?"` is false, so the `N blocking | M non-blocking` terms were always omitted — which is the whole feature #71 added and [sei-load#97](sei-protocol/sei-load#97) was opened to pilot. 2. **The notice was wrong about the driver.** It announced that the driver "predates check.json's blocking and non_blocking fields" against `v0.11.0`, which was cut specifically to add them. 3. **Both disagreement warnings were dead code.** They are guarded by `is_count "$blocking"`, so neither could fire. That is the only automated cross-check between the verdict comment and the check run — and it was dark exactly as a clean conclusion becomes able to post an approval. ## Scope Four characters of path, plus a comment recording why the root read was wrong so nobody re-derives it. `.conclusion` was already correct and is untouched. The sentinel still works for a genuinely older driver: absent counts answer `null` at the nested path too, so `is_count` rejects them and the shorter line still prints. ## Verification Both jq invocations above were run against the driver's real output. `actionlint` reports the same 5 pre-existing findings as the base (lines 326/1001/1183); none from this change. YAML parses. Found by a platform review of the pipeline; the driver-side companion is [sei-internal-skills#399](sei-protocol/sei-internal-skills#399). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…a redundant label (#75) ## The verdict now reaches the request A requested review opens with 👀 and then says nothing on the request itself. The person who asked has to open the run, or scroll for the verdict comment, to learn how it ended. It now reacts on the triggering comment: | position recorded | reaction | |---|---| | `APPROVE` | 👍 | | `REQUEST_CHANGES` | 👎 | | none | *(nothing)* | Together with the 👀 the job opens with, the pair separates a review still running from one that decided. **Keyed on the position, not on the job succeeding.** A run that finishes without reading the diff is not an approval and must not wear one — that is the credential-outage shape, where the workflow goes green and the review read nothing. A conclusion that records no position gets no reaction, which is the honest answer: the verdict comment carries what it found. Comment path only — an automatic review has no comment to react to — and never fatal, like the 👀 before it. The `ai-review` tooling this replaced did the same thing: > Explicit requests receive a best-effort 👀 reaction while the review runs and > 👍 when it completes successfully. This restores the second half, with the trigger narrowed from "the job completed" to "the review took a position". ## A redundant label on inline findings Inline findings render as: ``` **review · suggestion** — the error message implies amm has no knobs ``` Every one of them is a review comment on a review's own pull request, so the word says nothing the surrounding context does not. The severity leads now: ``` **suggestion** — the error message implies amm has no knobs ``` Nothing matches on the removed text. Threads carry forward on the `<!-- seidroid-review:finding -->` HTML marker, which is untouched. ## Verification `actionlint` reports the same 5 pre-existing findings as the base (lines 326/1001/1183); none from this change. YAML parses. The reaction needs a live comment-triggered review to confirm, since it depends on `needs.guard.outputs.comment_id` being populated — the same path that proved the 👀 on [sei-load#96](sei-protocol/sei-load#96 (comment)). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Implements **PLT-1148**. seidroid review is now only invokable on the
repository
the pull request is on.
`@seidroid review owner/name#123` reviewed a pull request in another
repository.
`ai-review.yml` never supported this — every call there uses
`context.repo` — so
this is net-new capability the rewrite introduced, withdrawn by
decision.
## What went
31 expression occurrences of `review_repo` / `review_repo_name` /
`review_pr` on
27 lines, across 11 steps plus the guard's `outputs:` block and the
review job's
`concurrency.group`. Plus `target_re`, the guard's target-parse block,
the dead
`REPO_OWNER` env entry, the `Place findings on the code`
identity-or-same-repo
condition (a tautology once there is no target), and 17 prose sites.
**No `workflow_call` input changes**, so neither caller breaks.
## What replaced it
An explicit refusal. `ai-assistant.yml` reserves only the exact body
`@seidroid review`, so the withdrawn form would otherwise fall through
and get a
conversational answer — a worse signal than silence, because it looks
like the
system worked.
## A bug found and fixed inside this change
The refusal's first cut read a `grep -q` exit status. Under `set -o
pipefail`
that is wrong: `-q` exits at the first match, `printf` is then killed by
SIGPIPE
writing into a closed pipe, and `pipefail` propagates 141 — so **the
refusal is
silently not written**. Measured: at 232 kB the pipeline returned 141
and the
refusal was missed.
The threshold is a race between `printf`'s write and `grep`'s
read-then-exit, so
it passes every small-body test and fails nondeterministically on a long
comment. GitHub's comment cap is 65,536 characters, so the input is
reachable.
Fixed by capturing through `grep -m1 … || true` and testing for
emptiness — the
same idiom the `cmdline=` line three lines above already uses.
Re-measured
through the extracted script: the refusal fires at 1 kB, 65 kB and 200
kB, one
`::error::` and one `should_run` key each time.
## Two traps avoided
**`repositories:` stays on both App-token mints.** With `owner` set and
`repositories` absent, the token covers every repository the
installation
reaches — a privilege widening disguised as cleanup.
**The regex target group and the parse block go in the same commit** as
the
expression collapse. A half-removal parses a target, admits it, and
reviews the
*local* pull request while the requester believes otherwise.
## Verification
```
grep -nE 'review_repo|review_pr|target_re|REPO_OWNER' empty
grep -niE 'cross-repositor|asking repositor|elsewhere' empty
actionlint 5 findings → 4; SC2129 gone (it sat on a deleted line),
4×SC2102 unchanged at identical script offsets, nothing new
shellcheck clean on the extracted parse script
step bodies 2 changed (parse, Post the verdict), 11 byte-identical
```
20 primary behavioural rows plus 12 supplementary, driven through the
extracted
`parse` script. Every row required to stay identical is byte-identical
to the
base — including a CRLF body, tab separators, uppercase, backtick-quoted
mentions, and both malformed-target shapes.
One drift row beyond the four expected: a body carrying **both** a
target line
and a valid bare command line. The base matched the target; this matches
the
bare line and runs an ordinary local review with no annotation. That is
the
right precedence and the honest signal.
## Not changed, worth a look
**The refusal annotates but does not fail.** `::error::` does not fail a
step, so
the guard still concludes `success` and the requester may never open the
run —
while `ai-assistant.yml` still answers the target form conversationally.
Widening that reservation would put the refusal where the request was
written.
Out of scope here.
**`github.repository` in the `concurrency.group` is now redundant** — a
reusable
workflow's group is already scoped to the calling repository. Kept,
because
dropping it would change a live group key for any in-flight run.
## Pre-merge check
The driver keys a session on the reviewed `(repo, pr)` pair, so a
session created
by a cross-repo review becomes unreclaimable through this workflow — and
the
launcher sets no lifetime cap while the server runs no sweep. Enumerate
live
sessions for a key naming anything other than a caller's own repository,
or
search run history for the guard's `::notice::reviewing <target>`
annotation, and
close any found **before** a caller's pin moves.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Implements **PLT-1144**.
The job adds an `eyes` reaction to the triggering comment and never
removes it.
The `stale` sets were `+1`, `-1`, `+1 -1` — `eyes` appeared in none. A
finished
review therefore reads as still in progress, permanently, and on a
conclusion
that earns no thumb there is no completion signal at all.
`ai-review.yml` removes it on every path.
## The change
Three code lines:
```sh
failure) reaction="-1"; stale="+1 eyes" ;;
success) reaction="+1"; stale="-1 eyes" ;;
*) reaction=""; stale="+1 -1 eyes" ;;
```
The rest is comment accuracy on lines this makes stale. Two of those
corrections
are worth naming, because each described a guard as something other than
what it
does — the class of defect five review rounds on this step kept finding:
- The step header claimed the condition names "a verdict to report". It
does
not; it names a comment to answer. The next line already said there is
no
`verdict_produced` gate, so the block contradicted itself, and a reader
trusting the first sentence would add the gate back.
- The absent-check-file comment named only a stale thumb as what an
early return
would strand. The eyes is the larger loss.
## Behaviour, measured
The extracted step driven against a `gh` stub that runs the workflow's
**own**
`--jq` filter, so the filter is under test. 25 runs: five starting
states × five
conclusion inputs.
| Start | success | failure | neutral / empty / missing |
|---|---|---|---|
| bot eyes | del eyes · post 👍 | del eyes · post 👎 | del eyes · post
none |
| bot eyes + stale 👎 | del both · post 👍 | del eyes · post 👎 | del both
· post none |
| human 👎 only | del none · post 👍 | del none · post 👎 | del none · post
none |
| nothing | del none · post 👍 | del none · post 👎 | del none · post none
|
| bot eyes + bot 👍 + two humans | del eyes · post 👍 | del eyes, 👍 · post
👎 | del eyes, 👍 · post none |
Every run exits 0. **No human reaction is deleted in any of the 25.**
Degraded paths all exit 0: a failed listing warns and still posts; a
failed
delete warns per reaction and still posts; a failed post leaves the
withdrawals
standing.
## Why the table is not vacuous
The first harness was wrong and passed everything **including the base**
— an
assertion anchored on `$` against a log line with a trailing space.
Corrected,
then mutation-tested after committing:
| Mutant | Result |
|---|---|
| base `a32defa` | eyes survives all 15 — the regression reproduces |
| head | clean |
| `eyes` off the success arm | caught |
| `eyes` off the `*` arm | caught |
| `select(.user.login == $me)` removed | caught — deletes the humans'
reactions |
| early return on an absent check file | caught |
The last two are the guards that must not break, and the harness proves
it would
notice.
## Verification
```
actionlint 5 findings, rule-for-rule and message-for-message identical to base
(4×SC2102 at in-script 23:12, 24:12, 175:14, 176:14; 1×SC2129 at 85:3)
shellcheck clean on both reaction steps
pipelines none in the step; pipefail has nothing to trip
```
## Out of scope, filed separately
A **cancelled** run leaves the eyes on. `!cancelled()` skips this step
when the
concurrency group cancels the run, so two `@seidroid review` comments in
quick
succession leave the first wearing eyes with no answer ever coming.
`ai-review.yml` avoids this by clearing in a separate job under
`always()`.
Changing when a step runs on cancellation is past this ticket.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… reaches no verdict (#85) Closes PLT-1143 (workflow half). The driver half shipped in sei-protocol/sei-internal-skills#405 and releases as `v0.14.0`. ## Problem A review that reaches no verdict publishes nothing. The pull request carries no comment, no check run, and no reaction that says anything happened. A reviewer sees a red job and an untouched pull request, and cannot tell a review that ran and could not be read from one that never ran. Only one of the two is a reason to look. ## What changes **The check run publishes whichever check the driver wrote.** The step dropped its `verdict_produced` gate; the file's presence is the gate instead. A no-verdict run writes a check under the title `no verdict` concluding `failure`, carrying the reason and no `counts` key. A run that never reached the driver still publishes nothing. Read the **title**, not the conclusion, to tell the two apart — a decided review carrying blockers concludes `failure` too. **A new step reports the run in a comment.** It is the exact complement of `Answer the request`, on one output: `== 'true'` there, `!= 'true'` here. So a review run that was not cancelled posts one comment or the other, never both and never neither — including a run that died before the driver, where `verdict_produced` is unset and this is the only record left. The comment quotes the check run's own summary, so the two cannot disagree. When the run stopped before the review started there is no reason to quote, and the body says that rather than implying the review ran. **The position step keeps its gate.** `verdict_produced` is load-bearing there twice over: the withdrawal at the end reads the driver's finding counts, the no-verdict check carries none, and the no-counts branch proceeds by design. An ungated no-verdict run would clear a standing block on the strength of a review that produced nothing. The check run and the comment publish without the gate; anything that clears a merge gate keeps it. ## Parity `ai-review.yml` answers the same case the same way, and forces `failure` on unreadable output for the same reason. ## Requires Driver `v0.14.0`. On an older pin the check file is absent on a no-verdict run, the publish step self-gates on `[ ! -s "$CHECK" ]`, and the comment step still reports — so this degrades rather than breaks. ## Verification ``` python3 -c "import yaml; yaml.safe_load(...)" parses actionlint base 4 [shellcheck] actionlint head 4 [shellcheck] unchanged grep review_repo\|review_pr 0 (the cross-repo grammar #83 removed stays removed) ``` Not verified from here: whether the new step's condition fires as written in GitHub's runner. That needs a live no-verdict run, which the next `@seidroid review` after the pin bump will produce. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
`driver-version` defaults to `v0.14.0` and is no longer required, and
the install step now fails a driver it cannot drive instead of finding
out mid-review.
## What changed
- `driver-version`: `required: false`, `default: 'v0.14.0'`. Both
callers can drop the line and run on the default. Neither caller is
edited here — that is the follow-up, and until they do it, nothing about
their runs changes: both pin `uses:` by sha.
- The install step reads what `go install` resolved from `go version
-m`, then makes two assertions and exits 1 with a named `::error::` on
either.
- `GOTOOLCHAIN: auto` on the install step. The `setup-go` comment above
already says this step sets it; it did not. That comment is now true.
## The contract assertion, and why this one
The input's own objection to a default was that a default ages silently
against the subcommand and flag names this file uses. Two directions of
drift, two checks.
**Too old — a version floor equal to the default.** A caller may pin
ahead of the default, never behind it. This is what makes the conclusion
vocabulary single-valued while the callers still carry their own pins.
Measured: v0.11.0 concludes `neutral` where v0.12.0 concludes `success`,
and v0.14.0 writes a `failure` check for a no-verdict run where v0.13.0
writes none. Comparison is semver on the version triple, with a
pre-release below its own core — so a sha pin sorts against the floor as
the tag it follows.
**Too new — the flag surface, from `review --help`.** The subcommand
must exist and must accept every flag this file passes.
**What I deferred: the driver declaring a contract version**
(`sei-agent-driver contract` printing an integer). It is the stronger
answer — it is the only one that catches a renamed `SEIDROID_*` variable
or a dropped `check.json` field, neither of which `--help` can see. It
needs a Go change in `sei-protocol/sei-internal-skills` and a release
before this file could depend on it, which chains this ticket behind
another repository, and the integer itself is a new public contract.
Separate PR. Until then those two failures stay where they are today: at
run time.
I measured that the run-time floor holds — an unknown subcommand exits 3
rather than succeeding silently — so the deferred surface fails loudly,
just late.
## Verification
`actionlint` 1.7.12, `.github/workflows/seidroid-review.yml`: **5
findings before, 5 after**, same rules (4x SC2102, 1x SC2129), all
pre-existing and none in the changed lines.
`python3 -c "import yaml; yaml.safe_load(...)"`: parses.
The install step's own `run:` block, extracted from the file and
executed against a real `go install` of each version, with
`MIN_DRIVER_VERSION=v0.14.0`:
```
driver-version=v0.14.0 exit=0
installed sei-agent-driver v0.14.0
driver-version=v0.13.0 exit=1
::error::driver contract: sei-agent-driver v0.13.0 is older than v0.14.0, and reaches a different conclusion for the same findings; drop driver-version from the caller to take this workflow's own default
driver-version=v0.11.0 exit=1
driver-version=v0.10.4 exit=1
driver-version=09ee41de67699839b35f91b45f0f635a009ffa07 exit=1
installed sei-agent-driver v0.10.5-0.20260901214354-09ee41de6769
```
The last is the revision `sei-internal-skills` pins today. The flag
surface alone does not catch it: v0.10.5-pre, v0.11.0, v0.12.0, v0.13.0
and v0.14.0 all expose the identical `review --help`. The floor is the
only check that separates them.
The surface branches, against v0.14.0 and against two stubs:
```
bin014/sei-agent-driver exit=0
fake/renamed-flag exit=1
::error::driver contract: sei-agent-driver v0.99.0 does not accept `review` --check-out, which this workflow passes
fake/no-subcommand exit=1
::error::driver contract: sei-agent-driver v0.99.0 has no `review` subcommand, which this workflow invokes
```
Semver comparison, against the function text as committed, floor
`v0.14.0`: `v0.13.0` FAIL, `v0.14.0` PASS, `v0.14.1` PASS,
`v0.14.0-0.<ts>-<sha>` FAIL, `v0.14.1-0.<ts>-<sha>` PASS, `v0.15.0`
PASS, `v1.0.0` PASS, `dev` FAIL, empty FAIL.
**Not verified from here:** that GitHub applies the default when a
caller omits the input, and that the `::error::` annotation renders as
one line in the runner. Both are GitHub behaviour, not testable locally.
## Two things the input description had wrong
Both were load-bearing, and both are corrected.
1. It said `go install` consumes the path-prefixed tag. It does not:
`@sei-agent-driver/v0.14.0` is rejected as `invalid version: ...
disallowed version string`. The bare `@v0.14.0` is what resolves.
`sei-load` already passes the bare form.
2. It said every tag predates the `review` subcommand. `v0.10.4` exposes
`xreview`; `v0.11.0` onward exposes `review` with the full flag set this
file passes.
## Out of scope
Deleting the input. The withdrawal gate's logic. Editing `sei-load` or
`sei-internal-skills`.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…the dismissal (#88) The step that records the review's position required the App identity, so a repository with no `SEIDROID_APP_ID` got a verdict comment and never appeared in the reviewers list. It now falls back to `github.token`, which is what every sibling publisher in this file already does. ## What changed - `State the review's position on the pull request` drops `steps.identity.outputs.token != ''` from its condition and reads `steps.identity.outputs.token || github.token`. The App still authors the position wherever a caller configured one. - The position post gains a `COMMENT` rung, mirroring `.github/workflows/ai-review.yml:983-986`. When the API refuses `APPROVE` or `REQUEST_CHANGES`, the step retries the same body as `COMMENT` and warns. - Two comments now state the present: the position step names what the fallback identity costs, and the `Answer the request` step names why it stands apart from the position step. The withdrawal, its interlocks and its loud failure on a stuck block are untouched. ## What a GITHUB_TOKEN review can and cannot do **Measured in this repository.** `github-actions[bot]` — the login `GITHUB_TOKEN` carries — has created `APPROVE` reviews here through `ai-review.yml`'s identical fallback: PRs #84, #83, #75, #74, #73, #72, #71, #70 and #61 all carry a `github-actions[bot]` `APPROVED` review whose body opens with `<!-- ai-combined-review -->`. It has also created `REQUEST_CHANGES` reviews and dismissed them: the timeline of #75 records two `review_dismissed` events with `actor=github-actions[bot]`, `state=changes_requested`, message `Superseded: latest AI review found no blocking issues.` So the create path and the dismissal path both work under this repository's settings. That the approvals land at all means sei-protocol allows Actions to approve a pull request. GitHub refuses the call outright when that setting is off, and none of these calls was refused. **Cannot.** A `COMMENT` review casts no vote. Where the rung fires, an `APPROVE` clears no approval requirement and a `REQUEST_CHANGES` blocks no merge. The check run remains the gate in both cases. ## The branch-protection question **Neither identity clears the review requirement on `sei-chain` today.** sei-chain PRs #4094 and #4100 each carry exactly one review — `seidroid[bot]` `APPROVED`, from the App — and GraphQL `reviewDecision` on both reads `REVIEW_REQUIRED`. sei-chain's `main` is protected and carries `.github/CODEOWNERS`. So a bot approval, App-authored or not, does not satisfy that gate now. The fallback costs nothing there that the App identity was buying. `uci` itself has no gate to satisfy. `main` reports `protected: false`, and the one ruleset (id 7955617, `~DEFAULT_BRANCH`) is `enforcement: disabled`. Were it enabled it would ask for `required_approving_review_count: 1`, `required_reviewers: []`, `require_code_owner_review: false`, `dismissal_restriction.enabled: false` — a rule that names no reviewer and restricts no dismissal. ## What I could not verify - **Whether a `github-actions[bot]` approval satisfies a required-approval rule.** `GET /repos/sei-protocol/uci/actions/permissions/workflow` and the org equivalent both returned 403 for my token, so I could not read `can_approve_pull_request_reviews` directly — the successful approvals above are the inference. No repository in reach has a merge gate that an Actions approval has ever been put to. - **sei-chain's protection detail.** `GET /repos/sei-protocol/sei-chain/branches/main/protection` returns 404 for my token. CODEOWNERS is the probable reason `reviewDecision` stays `REVIEW_REQUIRED`, not a measured one. - **A live run.** Nothing here ran on a runner. The evidence is the incumbent's history, the API state above and the local battery below. - **The `COMMENT` rung under a real refusal.** I never saw the API refuse a position, so the rung is exercised against a stub, not against GitHub. ## Two consequences worth naming 1. A repository with no App now runs the withdrawal where it previously skipped the whole step. A dismissal the API refuses fails the job, by the design this step already states. That is the intended loud failure, and it is new exposure for those repositories. 2. If the first post lands server-side but the client reports a failure, the `COMMENT` retry writes a second review. `ai-review.yml:968-991` carries the same hazard; the cost is one extra review, never a changed gate. ## Verification `actionlint` 1.7.12, against base `543f11f`: ``` base: 4 findings, exit 1 -> 4 SC2102:info new: 4 findings, exit 1 -> 4 SC2102:info rule set diff: identical ``` Both findings pre-date this change and sit in steps it does not touch. ``` $ python3 -c "import yaml; yaml.safe_load(open('.github/workflows/seidroid-review.yml'))" yaml ok $ shellcheck -s bash <the position step's script> (clean) ``` **Behaviour battery.** I extracted the step's script from the YAML at both revisions, put a stubbed `gh` on `PATH`, and ran twelve paths against each: event accepted, event refused, every event refused, approve off, blocking, blocking with the event refused, dismissals refused, review list unreadable, neutral with a blocker, neutral with nothing written down, neutral with no counts, and no check file. The base and the new step produce byte-identical output on every path but the two where the new `COMMENT` rung fires: ``` --- clean, approve on, event refused -::warning::could not record APPROVE ...; the verdict comment stands +::warning::... would not take APPROVE, so this review is recorded as a comment; + the check run carries the success conclusion --- blocking, request changes refused -::warning::could not record REQUEST_CHANGES ...; the verdict comment stands +::warning::... would not take REQUEST_CHANGES, so this review is recorded as a + comment; the check run carries the failure conclusion ``` Both loud failures still exit 1: the stuck dismissal and the unreadable review list. All four withdrawal interlocks are unchanged — a blocker beside a soft conclusion stops the withdrawal, a soft conclusion with zero counts stops it, absent counts let it proceed with a warning, and a `failure` conclusion exits before it. ## One thing the ticket got wrong The ticket's phrasing — "drop the identity requirement from the step's condition" — was written against a base that still carried the cross-repository target grammar. `bf507f3` removed that grammar, so the step now only ever posts to `github.repository` and dropping the requirement outright is safe. Against the older base it was not: `github.token` reaches no other repository, the withdrawal's review listing would have 404'd, and the step would have failed the job reporting a block it could not see. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sk for more (#86) The automatic path reviews every wired `pull_request` event. A caller that wires `synchronize` therefore spends a managed sandbox and a review's model quota on every push, with no input to stop it. `sei-load` wires `synchronize` today. ## What changed - A `re-review-on-push` input, boolean, defaulting to `false`. - A once-per-pull-request gate on the automatic path, in the guard's `admit` step. - `pull-requests: read` on the guard job, so the gate can read without the App identity. ## How the incumbent does it, and where I matched `ai-review.yml` carries the same input at lines 101-105 and the gate at lines 275-306. Its logic: - `alreadyReviewed` — any review by seidroid whose body contains `<!-- ai-combined-review -->` (lines 275-284). - `automaticReReview` — `action === "synchronize" && RE_REVIEW_ON_PUSH === "true"` (lines 294-296). - `shouldRun = ... && (!alreadyReviewed || automaticReReview)` (lines 297-300). - The explicit-comment path (lines 308 onward) never reads `alreadyReviewed`. I matched all four: | Incumbent | Here | |---|---| | Input `re-review-on-push`, boolean, default `false` | Same name, type and default | | Only `synchronize` bypasses the gate | Only `synchronize` bypasses the gate | | A comment request ignores the gate | The gate is inside the `pull_request` branch, so a comment cannot reach it | | `ready_for_review` reviews once, after the draft refusal | Unchanged draft check runs first, then the gate finds no verdict and admits | **One deviation: the artefact the gate reads.** The incumbent reads `pulls.listReviews`. That does not work here. This workflow posts a pull request review only when the conclusion is `failure`, or `success` with `approve-on-success` on — and `approve-on-success` defaults to `false` (line 105). It also needs the App identity (line 1067). So a clean review posts no review at all, `listReviews` would find nothing, and the gate would never engage. `ai-review.yml` posts one unconditionally, defaulting the event to `COMMENT` (ai-review.yml:952-970), which is what makes its signal reliable there. ## The signal I chose, and why **The verdict comment** — an issue comment on the reviewed pull request whose body opens with `<!-- seidroid-review -->`. - It is written for **every** review that reaches a verdict, clean or not (the posting step gates on `verdict_produced`, not on a conclusion). A run that reached no verdict leaves none, so it does not spend the pull request's one automatic review. - It **hangs on the pull request**, not on a commit, so a push does not remove it. - It needs **no new state**. The check run does not work. `POST /repos/{repo}/check-runs` takes a `head_sha`, so a push leaves the new head with no check run and every push would read as a first review. Finding all of them would mean walking the pull request's commits — O(commits) calls to answer one question. Matching is on the marker plus `user.type == "Bot"`, and the marker must **open** the body. The verdict posts under `seidroid[bot]` where an App is configured and `github-actions[bot]` where it is not, so the author is tested by type rather than by login. A person quoting the marker is not a Bot; a bot mentioning it mid-body does not open with it. ## Tracing the guard's outputs `deny` writes `admit=false` and exits 0, which is the path the draft and skip-review-label checks already take. `guard.outputs.should_run` becomes `false`, the review job's `if` (line 540-548) requires `'true'`, and the whole job is skipped. Nothing is left behind: the acknowledgement reaction, the check run, the verdict comment and the review position all live in the review job. The refusal writes one `::notice::` on the run and nothing else. The comment path is untouched, so the eyes-without-withdrawal shape cannot recur here. Ordering: the gate is the **last** check, so a draft, a non-member or a skip-labelled pull request is refused without paging the API. ## Verification `actionlint` 1.7.12, per file, before and after: ``` before after 3 ai-assistant.yml 3 ai-assistant.yml 4 ai-review.yml 4 ai-review.yml 22 release-check.yml 22 release-check.yml 4 release-publish.yml 4 release-publish.yml 5 seidroid-review.yml 5 seidroid-review.yml ``` Rule set on this file identical both sides: `4 SC2102:info`, `1 SC2129:style` — the same five pre-existing findings, no new one. `python3 -c "import yaml; yaml.safe_load(open('.github/workflows/seidroid-review.yml'))"` — parses. I extracted the `admit` step's script from the built YAML and ran it against a `gh` stub that replays comment fixtures through the real jq filter. 21 of 21 cases pass, covering all four acceptance criteria plus: the first automatic review still runs; a finding thread without a verdict does not count as a review; `reopened` and `labeled` still refuse with the input on (mirroring the incumbent); a verdict posted by `github-actions[bot]` counts; a human opening a comment with the marker does not; a failed read admits. The jq filter is separately proven under the real `gh` (gojq, not jq) against a live pull request: `$ENV` resolves, a real prefix matches, and a mid-body occurrence is rejected. **That live check caught a defect before it shipped.** `gh api ... -F per_page=100` with no `-X` switches the request to **POST** — the gate would have tried to create a comment on every push. The page size now rides in the path, and the test stub refuses a POST so a regression fails the matrix. **Not verified from here:** whether the guard's `if` and the review job's `if` actually evaluate as read in GitHub's runner. Job skipping, expression evaluation and the reusable-workflow permission check are not reproducible locally. Those rest on reading the file. ## Judgment calls for a reviewer 1. **`permissions: {}` becomes `pull-requests: read` on the guard.** Without it the gate needs the App identity, and a caller that configures no App gets no gate — the ticket's cost, unfixed. No working caller breaks: any caller that runs today already grants `pull-requests: write` for the review job, and a called workflow's permissions only narrow the caller's. To reverse, drop the grant and drop `|| github.token` from `GATE_TOKEN`. 2. **The read fails open.** A read that fails costs one extra review, which the next push corrects. A refusal on an unreadable signal costs the review itself, silently, on a pull request whose author never learns it was refused. This matches the skip-label check's own reasoning. 3. **No action allowlist.** The incumbent restricts automatic runs to six actions (ai-review.yml:287-289). I did not add that: the once-per-PR gate already stops every action after the first, and an allowlist is a second behaviour change the ticket did not ask for. 4. **A known, bounded race.** Two pushes close together can both read before the first verdict posts, and both admit. The review job's `cancel-in-progress` group collapses them to one verdict, so the cost is one partial sandbox, not two reviews. The incumbent has the same race for the same reason — its signal is also only written at the end. ## The ticket got one thing wrong It calls the verdict comment "the existing sticky verdict comment". It is not sticky. Commit 38a1e0b made each verdict a **new** comment, and the posting step says why (line 1519-1532). That does not weaken the signal — for "has a review ever run" a fresh comment per review is if anything a stronger record — but the gate had to be written to match on any such comment rather than on one upserted one. ## Out of scope, as stated The sandbox lifetime cap, verdict-comment deduplication, and the caller repos. `sei-load` and `sei-internal-skills` need no edit to benefit: the default is `false`, so the gate engages as soon as they pin a ref containing this. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…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>
Every review run publishes its check run twice, as `review` and as `AI
Review`. Both publish sites carry both names. A caller can turn the copy
off.
## What changed
Three sites in `.github/workflows/seidroid-review.yml`.
**`Publish the review check run`** sends the same conclusion, title and
summary under both names. It reads the check artifact once, so the two
cannot differ. `review` is unchanged: same name, same conclusion rule,
same error posture.
**`Post the verdict`**, on the path where the verdict comment fails to
post, publishes its `failure` check run under both names as well. It
named only `review` before, so the copy kept the review's earlier
conclusion and a rule naming `AI Review` read green on a verdict that
never arrived.
**`publish-ai-review-check`**, a new boolean input, default `true`.
## The decision
The repository owner decided to publish both names (PLT-1152). A
branch-protection rule matches a check by its name, so only a check
called `AI Review` satisfies a rule that requires `AI Review`. A
repository that retires `ai-review.yml` under such a rule waits forever
on a check nothing publishes, and the rule reads as pending, not failed.
The rules on the calling repositories are org-level, and no token in
this session can read them. Publishing both names is the safe answer
under a rule nobody can see, so the input defaults to on. Delete the
copy once someone who can read those rules confirms that none names `AI
Review`.
`publish-ai-review-check: false` is for a repository that runs both
tools and knows its own ruleset. Both tools then publish `AI Review`,
GitHub lists only the check run that completed last, and this copy can
hide the incumbent's verdict. The switch does not remove that race. It
makes the race a choice, and the description says so.
## Error posture
| Site | `review` | `AI Review` |
|---|---|---|
| `Publish the review check run` | exits the step under `set -e`, and
`continue-on-error` decides the job, as today | warns, and the step
still succeeds |
| `Post the verdict` fallback | warns, as today | warns |
`review` publishes first at both sites. The copy can never be the reason
the primary fails to post. The missing-`head_sha` paths are unchanged:
the publish step fails the job, the fallback warns.
## The two-publisher collision
The ticket asked me to confirm this rather than assume it. Half of the
premise holds.
Two check runs of one name on one commit are legal. GitHub does not show
both. `GET /repos/{owner}/{repo}/commits/{ref}/check-runs` defaults to
`filter=latest`, which returns one check run per name per check suite.
The checks list on the pull request reads that default view.
Measured on `sei-protocol/sei-chain` commit `1a086bc`, app `codecov`,
check suite `91901537092`:
```
filter=all codecov/project x3 ids 101144306086, 101144546091, 101145910119
default codecov/project x1 id 101145910119, the latest completed_at
```
One name plus one app plus one commit therefore means that the later
publish supersedes the earlier one in the view that matters.
`sei-internal-skills` posts both `ai-review.yml` and seidroid under the
App slug `seidroid`: its `AI Review` check run on commit `530588c`
carries `"app": {"slug": "seidroid"}`. On that repository the two `AI
Review` publishes race for the name, and the checks list shows whichever
completed last. That measurement is why the input exists.
Duplicates do sit side by side across check suites. Commit `530588c`
carries `ai-review / Claude` nine times, one per workflow run.
## Verification
I extracted both steps with a YAML parser and ran them under `bash` with
a `gh` stub on `PATH`.
`Publish the review check run`:
| Case | Exit | Check runs posted | Result |
|---|---|---|---|
| both publish | 0 | `review`, `AI Review` | one conclusion, title and
summary on both; log reads `published check runs review and AI Review:
success — review: 0 blockers` |
| the copy fails | 0 | `review` | warning raised; log reads `published
check run review: success — ...` |
| `review` fails | 1 | none | the step exits, and the copy is not
attempted |
| no `head_sha` | 1 | none | `::error::the reviewed commit was not
recorded on ...` |
| no verdict | 0 | `review`, `AI Review` | both carry `failure`, and the
file says `success` |
| empty check file | 0 | none | `no check run to publish` |
| copy off | 0 | `review` | log reads `published check run review: ...`
|
| malformed check file | 0 | `review`, `AI Review` | title reads
`review`; on the base branch it is empty |
`Post the verdict`, the failure fallback:
| Case | Exit | Check runs posted | Result |
|---|---|---|---|
| the comment posts | 0 | none | `posted the verdict on ...` |
| the comment fails, copy on | 0 | `review`, `AI Review` | both
`failure`, both titled `review produced but not published`; the base
posts `review` alone |
| the comment fails, copy off | 0 | `review` | `failure`, as the base
does |
| the comment fails, no `head_sha` | 0 | none | `::warning::no reviewed
commit was recorded ...` |
The base branch's scripts exit the same way on all twelve cases.
The harness reads the workflow-level `env:` out of the file rather than
restating it. `#86` hoisted `VERDICT_MARKER` from the `Post the verdict`
step env to a workflow-level `env:`, and a harness that named the old
key modelled a step that no longer exists. All four `fb-` cases run
through the line that reads it.
`actionlint` over `.github/workflows/*.yml`, base `5f5fd78` and this
branch: 6 `action`, 30 `shellcheck` (1x SC1102, 25x SC2086, 4x SC2102),
1 `syntax-check`. Identical. Both new `gh api` calls sit inside shell
functions, so the `output[title]` and `output[summary]` literals stay at
four.
## Not verified
- Nothing ran on a GitHub runner. The stub proves the scripts; the API
did not.
- I read no org ruleset. Nobody in this session can read one. Whether a
rule names `AI Review` is still unknown.
- I measured the superseding behaviour on the list endpoint. I did not
create two check runs of one name and watch a branch-protection rule
resolve them.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…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>
A re-review now closes the thread whose finding it addressed or restated, instead of leaving it open beside a new copy. On one live pull request 5 of 13 threads were byte-identical repeats of two findings; a third re-review has the author read one finding in three places and dismiss it three times. This is the workflow half of PLT-1145. The driver half is sei-protocol/sei-internal-skills#407 and needs a `v0.15.0` cut. **Merge order does not matter** — see the last section. ## What changed **`Read the threads this review left before`.** The GraphQL query takes each thread's `id`, and the `jq` emits it as `thread_id`. The selection now tests the root comment's author as well as the marker. The author test is the missing half of "this tool's own thread". The marker alone admits any thread whose first comment *quotes* it — and on a repository holding this file, that is any review comment quoting the `MARKER:` line. Such a thread reaches the prompt today as "a finding you left", which is a place for a pull-request author to write text this tool attributes to itself. With resolution added it would also be a thread this tool can close. Both tests now have to hold. That file is the allowlist. The driver admits an id only when it matches a thread listed there, so what this `jq` selects is exactly what a review is permitted to close. **`Resolve the threads this review closed`**, new, last of the publishers. It reads `.threads` from the `check.json` the job already reads for the check run: - `.threads.addressed` — the finding is gone from the diff. Resolved whenever the review published. - `.threads.superseded` — the finding is restated as a new inline comment. Resolved only when `steps.place` put comments on the code **and left nothing unplaced**. A thread closed behind a comment that never posted takes a live finding off the pull request and puts nothing where it was. That gate is per review, not per thread, and the step comment says so. Neither the findings file nor `check.json` carries which comment replaces which thread, so one unplaced comment holds every superseded thread open. It errs toward a duplicate thread; the other way errs toward a lost finding. `${PLACED_UNPLACED:-1}` defaults to 1 on purpose — an absent output means placement did not report, which must read as "something may be unplaced", not as zero. - `.threads.refused` — ids the driver would not match to a thread it was handed. Echoed as a `::warning::` and never acted on. Last, so nothing old closes before the new review is on the pull request. `continue-on-error: true`, and every path exits 0: a review that ran and published is never failed over a thread it could not close, and the cost of not closing one is the duplicate this workflow leaves today. ## How `ai-review.yml` does it, and where this matches Read against `.github/workflows/ai-review.yml` on this branch. **Matched.** - The mutation, verbatim — `ai-review.yml:1064-1073`, `resolveReviewThread(input: {threadId: $threadId})`. - An id is admitted against a set built at publish time, immediately before the call — `ai-review.yml:1021-1057`. - An ineligible id warns and is skipped, and the loop continues — `ai-review.yml:1061-1063`. - A resolve failure never fails the review — `ai-review.yml:1074-1077`, the `catch` around the whole block. Here it is `continue-on-error` plus an exit 0 on every branch. - Superseding ids ride only on comments that posted — `ai-review.yml:995-1000`. - `reviewThreads(first: 100)` — same page size the history read already uses. Past that is PLT-1162. **Deviated, with the reason.** - **The ownership test.** `ai-review.yml:1013-1017` keys on reviews the bot posted carrying its marker, then matches each thread's root comment `pullRequestReview`. That does not transfer: `Place findings on the code` posts each finding with `POST /pulls/{n}/comments`, so seidroid's threads hang off implicit reviews with empty bodies and no marker. The equivalent here is the root comment's marker and author, which is what the history read already selects on. - **Validated twice, not once.** `ai-review.yml` has no driver, so it checks at the sink only. Here the driver refuses an id that matches no thread it was handed, and this step refuses one that is not, right now, an unresolved thread this reviewer wrote. A mutation on somebody's pull request driven by model output earns the check beside the call as well as upstream of it. - **Already-resolved is not a warning.** `ai-review.yml:1051` excludes resolved threads from the eligible set, so naming one warns. This tells them apart: a thread that is ours but already resolved logs a line, and only an id that is not ours warns. The refusals are read by a human, so a false alarm costs the real one its weight. - **The superseded gate reads three counters, not one boolean.** `steps.place.outputs` already reports `on_line`, `on_file` and `unplaced`. Stricter than `ai-review.yml:996`, which asks only whether the review posted its inline comments, and sourced from what this job already measured. ## The id validation, and what it refuses Two layers, and both must admit an id before anything is resolved. The driver refuses any id that does not match a `thread_id` in the file this workflow wrote — including every id when that read failed and the file is empty — and refuses anything outside the alphabet a GitHub node id uses. Those land in `.threads.refused`. This step then rebuilds the set from GitHub, minutes after the history read, and admits an id only when it is an unresolved thread whose root comment carries the marker **and** was written by this run's identity. Everything else is refused with a warning and left open. So a thread a human resolved during the review is not re-closed and reported as this tool's doing, and a thread this tool did not write is never touched. Refused, and exercised (below): an id no thread carries; a marker-quoting thread written by somebody else; a superseded id when nothing was placed; every id when the identity does not match the threads' author. ## Verification **`actionlint`, before and after — the rule set is unchanged.** Four `SC2102` on the base, four on this branch, and nothing else on either: ``` $ actionlint .github/workflows/seidroid-review.yml # origin/feat/seidroid-review …:1022:9: shellcheck reported issue in this script: SC2102:info:37:12: … …:1022:9: shellcheck reported issue in this script: SC2102:info:38:12: … …:1432:9: shellcheck reported issue in this script: SC2102:info:199:14: … …:1432:9: shellcheck reported issue in this script: SC2102:info:200:14: … $ actionlint .github/workflows/seidroid-review.yml # this branch …:1062:9: … SC2102:info:37:12: … (the same four, moved down the file) …:1062:9: … SC2102:info:38:12: … …:1472:9: … SC2102:info:199:14: … …:1472:9: … SC2102:info:200:14: … ``` **The `jq` selection, against a fixture.** Four threads: two written by the bot with the marker, one written by `mallory` whose body *quotes* the marker, one ordinary human comment. It selects the two, drops `mallory`'s, and falls back from a null `line` to `originalLine`. **The resolve step's shell, extracted and run with `gh` stubbed.** Nine cases, all exiting 0: | case | outcome | |---|---| | supersede 3, **all** replacements unplaced, 1 unrelated finding placed | **0 closed** — the case the tightened gate exists for | | supersede 1, its replacement placed, nothing unplaced | 1 closed | | comments placed, ids resolvable | 2 closed; the invented id warned; the already-resolved one noted, not warned | | nothing placed | `superseded` held back with a line saying so; `addressed` still closed | | the reply names `mallory`'s marker-quoting thread | refused, 0 mutations | | the thread listing fails | warning, 0 mutations, exit 0 | | the mutation fails | warning per thread, exit 0, review still published | | an older driver wrote no `threads` key | closes nothing | | no check file | closes nothing | | `REVIEWER_LOGIN` does not match the threads' author | everything refused, 0 mutations | | `REVIEWER_LOGIN` unset (the naming step failed) | everything refused, 0 mutations | | `steps.place` outputs empty or unset | `superseded` held back, `addressed` closed | **Against the driver's real bytes.** `check.json` written by the driver on sei-internal-skills#407 was fed to this step unmodified: the invented id was refused at both layers, the already-resolved id was a no-op, and the two live threads got the mutation call. **Not verified.** `resolveReviewThread` was never called against GitHub. It cannot be from a workstation — it needs a live pull request carrying a thread this tool opened. The stub records the call and its arguments; it does not prove the API accepts them. The `app-slug` output the identity check reads was confirmed to exist on `actions/create-github-app-token` at the SHA this file pins, by reading that SHA's `action.yml`. ## Merge order, and the one risk Either order works, and neither half waits. - Old driver, this workflow: no `threads` key, this step closes nothing, the review publishes as it does today. - New driver, old workflow: the driver writes a plan nobody reads. No flag is added, so nothing here touches the install step or `driver-version` — this does not conflict with #87 and does not depend on it. **The risk worth naming.** If `REVIEWER_LOGIN` does not match the login that wrote the existing threads, the history read carries nothing and no thread can be closed. That fails closed rather than open, and it is loud: the step warns naming the count, the login, and the consequence. It happens where a repository's earlier reviews ran on `GITHUB_TOKEN` and later ones on the app, or the reverse. The marker and the login are each one value now. `FINDING_MARKER` sits at workflow level beside `VERDICT_MARKER`, which `#86` hoisted there — two markers in two scopes is how a third gets defined somewhere else again. `REVIEWER_LOGIN` is exported once to `$GITHUB_ENV`, because an `env:` block is evaluated before any step runs and the app slug is a step output. `FINDING_MARKER` is deliberately not called `MARKER`: two steps still carry a step-level `MARKER` of their own — the finding marker and the no-verdict marker — and a step-level key shadows a workflow-level one silently, so a bare `MARKER` would work today and become a trap for whichever step later forgot to set its own. One site that looks like it should read `REVIEWER_LOGIN` and must not: `me="github-actions[bot]"` in *Answer the request*. That step sets `GH_TOKEN: ${{ github.token }}` unconditionally and never the App token, so it really does react as `github-actions`. Unifying it would leave stale eyes and thumbs on every request whenever App credentials are configured. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two literals, moved together as the floor's own comment requires: `driver-version`'s default and `MIN_DRIVER_VERSION`, both `v0.14.0` → `v0.15.0`. A caller may run ahead of the default, never behind it. ## Why now `v0.15.0` writes a `threads` object into `check.json`. The resolve step added by #90 reads it to close the review threads a re-review addressed or replaced. On `v0.14.0` that object is absent, the step finds nothing, and every finding that still holds gains a new thread beside the old one — which is the defect PLT-1145 exists to remove. The step degrades rather than failing, so this is inert capability rather than a broken run, but it is inert until this lands. ## Also updated Three comments that name a version by example or by contrast. The two `go install` examples now name `v0.15.0`, and the floor's rationale gains the `v0.14.0` → `v0.15.0` difference beside the two it already lists. Left alone: the sentences describing what `v0.11.0` through `v0.14.0` each concluded, which are facts about those releases and stay true. ## What this refuses Any caller pinning below `v0.15.0` now fails at install with a named message rather than mid-review. Both callers pin `uses:` by sha and still run an older workflow, so nothing breaks today — but the cutover must drop each caller's `driver-version` line **in the same commit** that bumps its `uses:` sha. That ordering is recorded on PLT-1165 and PLT-1170. ## Verification ``` yaml.safe_load parses actionlint base 4 SC2102 actionlint head 4 SC2102 unchanged grep v0.14.0 2 remaining, both deliberate ``` Not verified: nothing ran on a GitHub runner. The install step's floor comparison was exercised against real `go install`s when it shipped; this change moves its constant and does not touch its logic. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… 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>
An author can now ask for a polish pass. `seidroid-review.yml` never
passed
`--include-nits`, so `IncludeNits` was false on every review and the
driver's nit
setting was unreachable from a caller. This wires it to a label on the
reviewed
pull request, and adds the flag to the install step's contract check.
## Label, not a boolean input
The person who wants nits is the author of one pull request. A boolean
input keys
off the caller's configuration, so it turns nits on for every pull
request in the
repository or for none. A label is set per pull request, by the author,
without a
workflow edit.
`nitpick-label` matches `ai-review.yml`'s input of the same name and its
`ai: nitpick` default (`ai-review.yml:66-70`), so a repository running
both tools
adds one label rather than two. Its two consumers there are the prompt
(`ai-review.yml:783`) and the poster (`ai-review.yml:799, 838, 896`),
both fed
from one label read in the preflight resolve step (`ai-review.yml:268`).
Empty disables the check, which is `skip-review-label`'s semantics in
this file
rather than `ai-review.yml`'s.
One semantic does not transfer. `ai-review.yml` re-runs on a `labeled`
event when
the changed label is the nitpick one. This workflow reviews a pull
request once
and states that a relabel earns no second review, so adding the label
starts
nothing. The author labels the pull request and comments `@seidroid
review`. The
input says so.
## Where the read goes, and what it costs
Its own step in the review job, `Read the nit setting from the pull
request`,
immediately before the driver invocation. It costs one `GET
/repos/{o}/{r}/pulls/{n}`
on the review path only.
Not in the guard's `Admit the request`. The guard's skip-label read runs
under
`GH_TOKEN`, which is the App token with no fallback; a caller that
configured no
App would never be able to opt in. Answering that caller needs
`GATE_TOKEN`, which
is a separate `gh api` call whichever job it lives in — so sharing the
guard's call
would mean restructuring a fail-open/fail-closed admission check for a
signal that
decides nothing about admission. The guard also `deny`s by `exit 0`
mid-step, so an
output added after the label check is not written on a denied path.
Not inside the drive step either, which is the tighter constraint. That
step
deliberately carries no GitHub token, and its env reaches the driver
process. A
`GH_TOKEN` there would hand the reviewing agent a GitHub credential.
The read uses `any(.labels[]?.name; . == $ENV.NITPICK_LABEL)`, the shape
#93 gives
the guard's own label check. It answers a failed read differently, and
on purpose:
the skip label withholds work, so refusing on a signal nobody could read
is the
safe answer there; this label asks for advice, so refusing would spend
the review
to protect the polish pass. A failed read warns and leaves nits off. The
comment
says so beside the code.
## The install contract check
The install step reads `review --help` and refuses a driver missing any
long flag
this file passes, before a session opens or quota is spent.
`--include-nits` is now
on that list. Without it a driver that dropped or renamed the flag would
pass the
check and fail inside `Drive session + collect verdict`, after install
had already
admitted it — which is the failure the check exists to move earlier.
The list is confirmed complete against the argv the drive step actually
builds,
not against the list as written; see report 3 below.
## What turning nits on changes on the pull request
Off does not mean dropped. Read against `sei-agent-driver` at `v0.15.0`,
which is
both the `driver-version` default and `MIN_DRIVER_VERSION` after #95:
| | label absent | label present |
|---|---|---|
| a nit-grade observation | `nitRule` sends it to `non_blockers`: prose
in the verdict comment and in the check run's Non-blocking section, no
thread on the code | reported inline with severity `nit`: a comment
thread on the line |
| a nit the review placed inline anyway | dropped — `PlaceableFindings`
(`findings.go:128`), the counts (`findings.go:373` via `countFindings`),
and the check summary, which renders only the line-less buckets | placed
and counted |
| a prior thread a nit restates | supersedes nothing, because no comment
posts | superseded, and resolved once the comment is on the code |
So the label chooses where a nit lands, not whether the review makes
one. The
prompt states the current setting on both settings and says it replaces
an earlier
one (`prompt.go:527-548`) — load-bearing here, because the session
outlives the run
and a first turn told to leave nits out still holds that instruction.
## Verification
Nothing ran on a GitHub runner. Three step scripts — `Install the review
driver`,
`Read the nit setting` and `Drive session + collect verdict` — were
extracted from
the shipped file with a YAML parser and run under `bash` with stubs. The
harness
asserts each step's `if` and the `INCLUDE_NITS` wiring against the file,
so a
rebase that changes one fails the harness rather than passing it. The
`gh` stub
runs the shipped `--jq` filter through real `jq`; the `go` stub serves a
driver
whose reported version and `review --help` flag set the case controls.
**1. driver argv**
```
case nit step --include-nits drive rc
label present include_nits='true' yes 0
label absent include_nits='false' no 0
no labels at all include_nits='false' no 0
no labels key include_nits='false' no 0
near-miss labels include_nits='false' no 0
caller renamed it include_nits='true' yes 0
label read fails include_nits='false' no 0
label input empty skipped no 0
close mode skipped no 0
every input set include_nits='true' yes 0
```
`every input set` exists so the union of flags below is the whole
surface. Its argv:
```
review sei-protocol/uci 42 --out .../verdict.md --findings-out .../findings.json \
--check-out .../check.json --conversation-context .../threads.json \
--guidelines-file REVIEW.md --extra-instructions "be terse" --include-nits \
--trigger-id 999
```
Close mode: `review sei-protocol/uci 42 --close`.
**2. install contract check**
```
case version mode rc annotation
help names every flag v0.15.0 review 0
help drops --include-nits v0.15.0 review 1 ...does not accept `review` --include-nits
help drops --check-out v0.15.0 review 1 ...does not accept `review` --check-out
driver below the floor v0.14.0 review 1 ...is older than v0.15.0
below the floor, close v0.14.0 close 0
```
The stub help gives half the flags a cobra shorthand (`-x, --out
string`), so the
check is exercised against the shape its own comment says it must
tolerate.
**3. contract list against real argv**
Parsed out of the shipped install script and compared with the union of
long flags
the drive step actually built across all ten cases:
```
contract list: --out --findings-out --check-out --close --conversation-context
--guidelines-file --extra-instructions --include-nits --trigger-id
argv built: --check-out --close --conversation-context --extra-instructions
--findings-out --guidelines-file --include-nits --out --trigger-id
built but NOT in the contract list: none
in the contract list but never built here: none
```
Both directions are assertions, so wiring a flag without listing it, or
listing one
the workflow never passes, fails the harness.
The real `sei-agent-driver@v0.15.0` was installed from the proxy and its
`review --help` names exactly `--check-out --close
--conversation-context
--extra-instructions --findings-out --guidelines-file --help
--include-nits --out
--trigger-id` — the contract list plus `--help`.
The `--jq` filter was also run through `gh`'s own engine
(`github.com/cli/go-gh/v2/pkg/jq`
v2.16.0): label present `true`, absent `false`, empty array `false`, no
`labels`
key `false`, `ai: nitpicky` `false`, `AI: Nitpick` `false`.
`actionlint` 1.7.12 on the same path with the same invocation: base
`30f5c09`
gives 4 findings, all `SC2102:info`; this branch gives 4, all
`SC2102:info`.
`shellcheck -S info` on all three extracted scripts: clean. The file
parses under
`yaml.safe_load`; 19 `workflow_call` inputs.
Not verified: any live run, and the flag's effect on a real model turn.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… newest replies (#96) The thread history now reads every page, and the replies it carries are the newest rather than the oldest. Both were silent losses. This is the workflow half of PLT-1162. The driver half is sei-protocol/sei-internal-skills#410 and needs a `v0.16.0` cut. **Merge order does not matter** — this fetches more, the driver decides what it can afford, and neither depends on the other. ## What changed **Pagination.** The query asked for `reviewThreads(first: 100)` and stopped. A pull request reviewed enough times carries more threads than one page holds, and the ones past the cut were absent with nothing said — so a re-review could not see a finding it had already made there, and made it again. That is the duplicate thread PLT-1145 removed, reached by a different route. The query now takes `$endCursor` and returns `pageInfo`, which is the whole of what `--paginate` asks for, and `jq -s` folds the pages into one array. `--slurp` would do the same inside `gh`; `jq -s` asks nothing of the runner's `gh` version. **The comment window was the wrong end of the thread.** One `comments(first: 20)` served both the ownership test and the replies. On a thread with forty comments that handed over the *oldest* twenty, and the driver then showed the last three of those — replies 17 to 19 of 40, presented as the latest word on the conversation. The root and the recent comments are now two connections: ```graphql root: comments(first: 1) { nodes { id body author { login } } } recent: comments(last: 20) { totalCount nodes { id body author { login } } } ``` The root stays `first: 1` and cannot move: every ownership test reads it, and the marker has to open its body. The replies are `last: 20`. The root is excluded from them **by id**, not by position, so a thread short enough to carry its own root inside that window does not report the finding back as a reply to itself. **Truncation is reported.** `totalCount` rides along, so a thread with more comments than this reads says so: ``` ::warning::N of this tool's thread(s) on owner/repo#42 carry more than 20 comments; this review reads the 20 most recent of each and the older ones are not in its history ``` It is counted over the threads the history **actually carries**, both ownership tests applied — counting every marked thread would report a shortened conversation on a run that carried no history at all, which says nothing true about what the review is working from. The step also now logs how many threads it read across every page beside how many it carried, which is what separates "this pull request has no history" from "this run did not recognise its own identity". ## How `ai-review.yml` does it, and where this matches **Matched.** The cursor loop — `ai-review.yml:466-495` walks `reviewThreads(first: 100, after: $cursor)` on `pageInfo { hasNextPage endCursor }` until it runs out. Same traversal; `gh --paginate` performs it rather than a hand-written `do/while`, because this step is shell and that one is `actions/github-script`. **Deviated, with the reason.** - **No character budget here.** `ai-review.yml:554-564` truncates the assembled history to 120,000 characters in the step that fetches it. This step hands over everything and the driver spends the budget (sei-internal-skills#410), because the driver is what renders the prompt. A cap here would be a second, silent one underneath a bound that already reports itself. - **Per-thread comment pagination is not done.** ai-review gets comment bodies from a paginated REST `listReviewComments` and uses GraphQL only for thread metadata (`ai-review.yml:461-463`). Doing the same here would mean a nested cursor loop per thread. Instead the window moved to the end that matters and the overrun is reported. Named as a deliberate limit rather than a silent one. ## Two invariants this had to preserve, and does - **The two-login read.** The history admits this run's identity or `github-actions[bot]`; the resolve step keeps the strict single-login test. Pagination did not touch that split — verified across pages, since the fixture's second page holds a `github-actions[bot]`-written thread. - **`startswith`, never `contains`.** Counted against the shipped file: `contains($ENV. …)` **0**, `startswith($ENV. …)` **9** (was 8; the new reply-truncation select is the ninth). ## Verification `actionlint`: 4× SC2102 on the base `41ee3ffa`, 4× on the branch, nothing else on either. YAML parses. The step was extracted from the built file and run against a two-page fixture with `gh` stubbed to emit the pages exactly as `--paginate` does — one JSON document per page, concatenated. | case | result | |---|---| | App identity, both pages | `read 5 review thread(s) across every page; carrying 3` — ids `[PRRT_ours1, PRRT_ga, PRRT_chatty]` | | workflow token only | carried 1 — only the `github-actions[bot]` thread | | neither login matches | carried 0, login-mismatch warning | | both logins unset | carried 0, login-mismatch warning | | ai-review comment quoting the marker mid-body | excluded, on both pages | | human comment quoting the marker | excluded | `PRRT_ga` sits on **page two** and is written by `github-actions[bot]`: before this change it was invisible twice over. Reply windowing, on a 41-comment thread: ``` replies: 20 first: alice: reply number 21 last: alice: reply number 40 ``` The old `first: 20` window gave replies 1-19, of which the driver showed 17-19. The reply-truncation warning fires for the 41-comment thread and, once scoped to carried threads, does **not** fire on the runs that carry no history. **Not verified.** No real page boundary was crossed. The fixture reproduces the two-page shape `gh --paginate` emits, but a live pull request carrying more than 100 review threads is not something I can produce from here — so the cursor traversal is verified against a recorded shape, not against GitHub. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Findings the diff can hold now travel in one `POST /pulls/{pr}/reviews`
call carrying the review body and every inline comment together. An
author with ten findings gets one review and one notification instead of
ten ungrouped threads, and the run spends one call where it spent ten.
## What changed
`Place findings on the code` posted one `POST /pulls/{pr}/comments` per
finding and let the API decide the rung by refusing. It now reads the
pull request's own diff first, splits the findings against it, and posts
the anchorable ones as a single review.
The three rungs hold, and the middle one still costs a call of its own:
the reviews API carries `path`, `line`, `side` and `body` per comment
and has no `subject_type`, so a file-level comment cannot ride in the
batch.
| Rung | Before | After |
|---|---|---|
| on the line | `POST .../comments`, one per finding | one `POST
.../reviews` for all of them |
| on the file | `POST .../comments` with `subject_type=file` | unchanged
|
| in the summary | appended to `$NOTE` | unchanged |
## How `ai-review.yml` does it, and what transferred
- `.github/workflows/ai-review.yml:860-880` walks each `pulls.listFiles`
patch into a set of commentable lines per file and side. RIGHT takes an
added or a context line, LEFT takes a removed or a context one.
**Transferred**, as a jq walk over the same patches. It is what makes
the batch safe to send.
- `ai-review.yml:897,901` anchors a finding whose line is in that set
and orphans the rest into the body. **Transferred in shape**: an
unanchorable finding here takes the file rung first and the summary
second, which is this workflow's own ladder and is richer than
orphaning.
- `ai-review.yml:960-967` builds the `comments` array and one
`createReview` call carrying `commit_id`, `event`, `body` and
`comments`. **Transferred.**
- `ai-review.yml:968-991` falls back inline → body-only → COMMENT. **Not
transferred as written.** That chain protects a review body this step
does not own: the verdict and the summary go out as an issue comment
from `Post the verdict`, and the position goes out from `State the
review's position`. The fallback here is different and is answer 4
below.
## The four points
**1. The finding marker.** Every inline comment in the batch opens with
`FINDING_MARKER` as its first bytes, built in jq as
`"\($marker)\n**\(.severity)** — \(.detail)"`. The review's own body
carries no marker. The history read and the resolve step both key on
`(.comments.nodes[0].body) | startswith($ENV.FINDING_MARKER)` over
review *threads*. A comment sent in a `createReview` call opens a thread
whose first comment is that comment, which is measured rather than
assumed: the same GraphQL query those two steps run, against #90,
returns five threads whose `comments.nodes[0]` is a `github-actions`
comment belonging to an `APPROVED` review — `ai-review.yml`'s own batch.
A marker on the review body would be read by nothing and is left off.
Case 1 in the table below asserts the marker is the first bytes of all
four comment bodies; case 3 asserts the same for the seven bodies the
per-finding fallback sends.
**2. `unplaced` on the merge gate.** What counts as placed is unchanged.
`on_line` still counts findings the API accepted, and it is incremented
only after the review call returns success. `on_file` and `unplaced` are
untouched. `Resolve the threads this review closed` reads `placed =
on_line + on_file` and requires `unplaced == 0`; the harness shows base
and branch agreeing on all three counts in every case where the API
cooperates.
One case moves, and it moves toward holding the gate: when the batch
fails with a 5xx or with no status at all, all of its findings go to the
summary, so `unplaced > 0` and no superseded thread closes. Base would
have placed those findings one at a time.
**3. Riding on the position step's call.** Rejected; this is a second
review object. Four reasons.
- `State the review's position on the pull request` records no review at
all on a `success` conclusion with `approve-on-success` false, or on
`neutral`. Those runs have findings and no call to ride on.
- It runs after placement, and the counts are `steps.place.outputs.*`.
Posting there would make placement report intent rather than outcome,
and the resolve gate would close threads on the strength of comments
that had not been sent.
- It carries no `continue-on-error`, because it holds the only
withdrawal of a standing block. An all-or-nothing comment batch on that
call lets one bad line cost the position and the withdrawal.
- The count of review objects goes down, not up. Measured on this
repository: `GET /pulls/{n}/reviews` on #88, #89 and #90 returns one
`COMMENTED` review with `"body": ""` per standalone comment. Ten
findings are ten review objects today and one after this.
The ticket says the position step is skipped on a no-verdict run while
placement is not. That is not true on this base: both gate on
`inputs.mode == 'review' && !cancelled() &&
steps.drive.outputs.verdict_produced == 'true'`, character for
character. The real asymmetry is the empty-`event` path above.
**4. The batch is all-or-nothing.** Two failures, two answers.
- **Any `4xx`.** The API refused the request and created nothing. Each
finding is posted on its own down the full three-rung ladder, so the one
comment that was refused costs only itself. This request carries every
finding's whole `detail`, which is model prose under no length bound, so
its size is refused as readily as its content and GitHub answers that
with `413`. Cases 3, 4 and 22 to 24 cover `422`, `413`, `403` and `400`;
case 4 refuses the batch and then one line individually, and that
finding lands on its file while the other three land on their lines.
- **Anything else,** including a call that reached no response. Those
findings go to the summary under a heading that says so. A 5xx, a
secondary rate limit or a dropped connection may be a write that landed,
and repeating it posts the review twice. The reader still gets every
finding, and `unplaced > 0` holds the superseded threads open. Cases 5,
6 and 25.
The code is read from the response's own status line, which `gh api -i`
puts first — not from a `status` field in the error body. GitHub's
validation-error schema does not declare that field, and a refusal
carrying none would have read as no refusal at all and sent every
anchorable finding to the summary, which is worse than base. Measured on
this endpoint: a 422 does carry `status` today, and the schema does not
promise it.
A refusal is predicted rather than met: the commentable-line index is
built before the call, from the diff **at the reviewed commit**. `GET
/pulls/{n}/files` answers for the pull request's current head and takes
no commit — measured, it accepts a `sha` parameter and ignores it, so a
push mid-review would index one commit and comment on another. `GET
/compare/{base.sha}...{REVIEWED_SHA}` does take one; measured on #90, it
answers differently per commit and reproduces `pulls/{n}/files` byte for
byte at the head. Its three-dot form is the diff the pull request shows,
and a base branch that moves during the review does not move that merge
base, because the reviewed commit is fixed.
The index is read as the diff only when its length matches the pull
request's own `changed_files`, which the same `GET /pulls/{n}` call
already answers. `GET /compare` sends at most **300** files and drops
the rest in silence: no total, no `Link` header for them, no flag. Its
pages are pages of commits, and a second page carries no `files` key at
all, so `--paginate` cannot reach the ones it dropped; it is gone from
the fetch, which now costs one call instead of one per hundred commits.
Measured against the live API. `kubernetes/kubernetes#137092` reports
`changed_files: 398`; `GET /compare/{base}...{head}` answers with
exactly 300 on a single page, while `GET /pulls/{n}/files --paginate`
returns all 398. Three-dot ranges of 309 and 321 files both answer with
300, and one of 251 answers with 251. `changed_files` and the compare
length agree on every whole list measured, including 295 and 261 — just
under the cap.
A short list read as the diff is the one thing the `unknown` bucket
exists to stop: every file it dropped looks exactly like a file the pull
request never touched, so each finding in one took a file comment whose
body told the author their cited line was outside a diff that holds it.
A short list now indexes nothing. Cases 18 to 21 cover a short list, a
list at the cap the count confirms, a list at the cap with no count to
confirm it, and a list under the cap with no count.
When the index cannot be built — the base commit or the diff cannot be
read — the step falls back to posting each finding on its own, which is
what it did before. Cases 10 and 14c.
A file the API sends without a patch is a third group, not a file with
no lines. A binary file and a file whose diff was too large both arrive
that way, so counting them as empty would drop every finding on one to
the file rung under a body claiming the cited line is outside the diff.
Those findings go to the API one at a time instead. Case 17.
## Verification
Nothing here ran on a GitHub runner. The step's script was extracted
from the shipped YAML with a YAML parser and run under `bash` with a
`gh` stub on `PATH` that serves fixture JSON through the step's own `jq`
and captures the request body. `base` is the same harness against the
same step extracted from `bc93b4f`.
Columns: API calls made, then the three counts written to
`$GITHUB_OUTPUT`, then lines in the summary note.
| # | Case | reviews | line-comments | file-comments | on_line | on_file
| unplaced | note |
|---|---|---|---|---|---|---|---|---|
| 1 | 4 findings, all on covered lines | **1** (base 4 comments) | 0 | 0
| 4 | 0 | 0 | 0 |
| 2 | + off-hunk line, untouched file, no line | **1** (base 7) | 0 | 3
| 4 | 2 | 1 | 1 |
| 3 | batch refused with 422 | 1 | 4 | 3 | 4 | 2 | 1 | 1 |
| 4 | batch 422, and one line refused on its own | 1 | 4 | 4 | 3 | 3 | 1
| 1 |
| 5 | batch fails 500 | 1 | 0 | 3 | 0 | 2 | 5 | 5 |
| 6 | call reached no response | 1 | 0 | 3 | 0 | 2 | 5 | 5 |
| 7 | zero findings (empty file) | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
| 8 | empty findings array | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
| 9 | findings file will not parse | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
| 10 | the diff cannot be read | 0 | 7 | 3 | 4 | 2 | 1 | 1 |
| 11 | no reviewed commit recorded | 0 | 0 | 0 | 0 | 0 | 7 | 7 |
| 12 | string line, junk line, missing side | 1 | 0 | 1 | 2 | 1 | 0 | 0
|
| 13 | empty severity | 0 | 0 | 2 | 0 | 1 | 1 | 1 |
| 14 | **index read at `REVIEWED_SHA`** | 1 | 0 | 0 | 4 | 0 | 0 | 0 |
| 14b | the same run against a pushed head's diff | 0 | 0 | 4 | 0 | 4 |
0 | 0 |
| 14c | base commit cannot be read | 0 | 7 | 3 | 4 | 2 | 1 | 1 |
| 15 | **500: on-diff findings get their own heading** | 1 | 0 | 3 | 0 |
2 | 5 | 5 |
| 15b | 422 ladder: what it believed on-diff, likewise | 1 | 4 | 7 | 0 |
0 | 7 | 7 |
| 15c | no on-diff group, so no on-diff heading | 1 | 0 | 3 | 4 | 2 | 1
| 1 |
| 16 | **422 whose body carries no `status`** | 1 | 4 | 3 | 4 | 2 | 1 |
1 |
| 17 | **file the API sent with no patch** | 1 | 2 | 1 | 2 | 1 | 0 | 0 |
| 18 | **list short of `changed_files`** | 0 | 4 | 0 | 4 | 0 | 0 | 0 |
| 19 | at the cap, the count confirms it | 1 | 0 | 0 | 1 | 0 | 0 | 0 |
| 20 | at the cap, no count to confirm it | 0 | 1 | 0 | 1 | 0 | 0 | 0 |
| 21 | under the cap, no count | 1 | 0 | 0 | 4 | 0 | 0 | 0 |
| 22 | **batch refused with 413** | 1 | 4 | 3 | 4 | 2 | 1 | 1 |
| 23 | batch refused with 403 | 1 | 4 | 3 | 4 | 2 | 1 | 1 |
| 24 | batch refused with 400 | 1 | 4 | 3 | 4 | 2 | 1 | 1 |
| 25 | batch fails 502 | 1 | 0 | 3 | 0 | 2 | 5 | 5 |
Cases 14 and 14b read `pull` and `compare` calls too; 14 asserts the
compare range is `{base.sha}...{REVIEWED_SHA}`.
Case 18 is the regression this round fixes: `pkg/b.go` is in the pull
request and missing from the compare response, which is what truncation
looks like. Read as the diff it puts the finding on `pkg/b.go` under a
body saying line 2 is outside a diff that adds line 2. Case 19 is the
case that must **not** bail — a genuine 300-file pull request whose
count agrees — so the guard cannot simply refuse any list of 300.
Every case exits 0. `continue-on-error: true` is unchanged on the step.
Base and branch produce identical counts in cases 1, 2, 3, 4, 7, 8, 9,
10, 11 and 14c. They differ in 5, 6, 15 and 15b by design (answer 4), in
14 and 14b because base has no index at all, and in 12, 13 and 17 for
the reasons below.
165 assertions over 29 cases pass, including: the request carries
`event: "COMMENT"`, the recorded `commit_id`, a non-empty body, four
comments in findings order with the right `path`/`line`/`side`, each
opening with the marker as its first bytes; a multi-line detail and one
carrying a backtick, a double quote and a `$` survive intact; the review
body starts with neither marker.
Also verified: `actionlint` finds the same four `SC2102:info` at the
same in-script offsets before and after, and the whole-repo output is
identical apart from line numbers. `shellcheck -S style` on the
extracted script is clean. The file parses as YAML. The harness now
lives at `test/seidroid-review/` and runs in CI from
`.github/workflows/workflow-test-self.yml`; it re-reads the step and the
marker out of the YAML on every run, so it cannot pass against a stale
copy.
Each guard was checked by removing it. Dropping the file-count test
breaks 6 assertions, narrowing the retry back to `422` alone breaks 10,
and widening it to 5xx breaks 13.
Not verified: nothing ran on a GitHub runner, and no call reached the
GitHub write API. The atomicity of `createReview`, the claim that no 4xx
can be a partial write, the 413 answer to an oversized body, and `gh`'s
placement of the error object on stdout are taken from the API
documentation and from HTTP semantics, not measured. The read side — the
300-file cap, the absent `files` key on page two, and `changed_files` —
is measured against the live API as above. Nothing here proves GitHub
accepts this exact payload; the first real run does.
The harness also caught two defects in this branch before it shipped.
The fixtures still modelled `pulls/{n}/files`, so every case fell to the
ladder and 46 assertions failed loudly rather than passing quietly. And
`gh api --jq` writes the error object itself on a failure, so a refused
base read left a line of JSON in `base_sha`; it is now tested for the
shape of a commit id, not merely for emptiness.
## Two things the harness found in the base
Both are in the record the fallback rungs read, and this change rewrites
that reader, so they are fixed here rather than left behind.
- **A field shift.** `@tsv` writes an empty field as nothing between two
tabs, and `IFS=$'\t' read` folds the pair into one delimiter because a
tab is IFS whitespace. A finding with no `side` therefore read one field
short: the severity arrived in the side, the base64 detail arrived in
the severity, and the detail was lost. On base, a finding with an empty
severity renders in the summary as ``- `pkg/untouched.go:3` () —`` with
no text at all. The normalising jq now guarantees every field but the
last is non-empty.
- **A line the model wrote as prose reaching `gh` as a field.** Base
passes `-F line="$line"` straight from the findings file, and `gh` reads
a leading `@` as a file to send. A finding whose line is `@/etc/passwd`
produced exactly that call. Lines are now coerced to an integer, and a
line that is not one reads as 0, which no diff covers, so the finding
takes the file rung.
## Scope
The 50-finding cap is not in this workflow; the driver writes
`findings.json` and bounds it. Thread resolution, the cap, and the
driver and caller repositories are untouched.
Residual: a diff so large the API sends no patch for a file a finding
names. That finding costs one line call before the API answers, which is
what base cost for every finding. A file present without a patch is
handled as unknown, and a file list short of the pull request's own
count now indexes nothing.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
`allow-policies` read as a control that narrows what the review agent
may do. It
was not one. Any value a caller set accepted every tool call.
`ai-review.yml`
has no counterpart, so this was net-new caller surface with no
capability behind
it. `allow-tools` is the input that discriminates, and it stays
untouched.
Implements **PLT-1158**. The diff is 13 deleted lines: the input
declaration and
the one env line that read it.
## The claim held, at the source
The ticket rests on one claim: `policy_name` is
`claude_native_permission` for
every native prompt, so any value accepts everything. Confirmed in the
omnigent
server. The value is a hardcoded literal with no branch on the tool:
```
omnigent/server/routes/sessions/routes_hooks.py:307-316
params = ElicitationRequestParams(
mode="form",
message=f"Claude wants to call **{tool_name}**",
requestedSchema=None,
url=None,
phase="pre_tool_use",
policy_name="claude_native_permission",
content_preview=f"{tool_name}({preview_str})",
**extras,
)
```
Read on the deployment branch `fork/feat/sei-base@ace8f922d`, fetched to
confirm
the remote tip, and on upstream `origin/main@381bf638f` at line 315.
That route
serves Claude Code's `PermissionRequest` hook, which is the path every
tool-call
prompt takes for the review agent. The gated tool identity rides beside
it as
the `tool_name` extra, and `tool_name` is what `allow-tools` matches.
The driver accepts on an exact match of `policy_name`:
```
sei-agent-driver/internal/driver/policy.go:141
case e.PolicyName != "" && p.AllowPolicies[e.PolicyName]:
return Accept, "policy_name allowlisted: " + e.PolicyName
```
The one value that can ever match a native prompt therefore accepted
every tool
call, rather than a class of them.
### Two limits on the claim, and why neither saves the input
A server-side inner policy stamps its own name through `deciding_policy`
(`omnigent/server/routes/_sessions/orchestration.py:2056`), so
`policy_name` is
not single-valued across the whole of omnigent. **The `seidroid`
bundle's own
guardrail config lives in the deployment and is not in the
sei-internal-skills
checkout, so I did not read it. An inner policy that asks under a
distinct name
is therefore not ruled out.** The generic native-permission route
(`routes_hooks.py:1374`) takes `policy_name` from the hook payload, so
on that
path the field is not even server-attested.
Neither limit argues for keeping the input. The prompts a review raises
are the
native ones, and every one of those carries the single literal. Adding
an input
back is additive and breaks no caller, so this is a two-way door.
## Nothing else reads the variable
```
$ git grep -n -I "allow-policies\|allow_policies\|ALLOW_POLICIES" # uci, after
(no match in any tracked file)
$ grep -rn "SEIDROID_ALLOW_POLICIES" ~/sei-internal-skills/ # outside the driver
(none)
$ grep -rn "SEIDROID_ALLOW" ~/omnigent/omnigent/
(none)
```
The driver reads it in one place, `cmd/sei-agent-driver/main.go:213`,
the same at
tag `sei-agent-driver/v0.15.0` that this workflow installs by default.
`os.Getenv` returns `""` for an unset variable, and `NewPolicy("")`
builds the
empty allowlist that today's `default: ''` also builds. Behaviour is
unchanged
for every caller. The warning at `main.go:216` still does not fire,
because
`allow-tools` keeps its `Bash,Read` default and its env line.
## No caller passes it
A reusable workflow refuses an input it does not define, so I checked
each site.
A code search finds two caller files and no third:
```
$ gh api search/code -f q='"seidroid-review.yml@" org:sei-protocol'
2
sei-protocol/sei-load .github/workflows/seidroid.yml
sei-protocol/sei-internal-skills .github/workflows/seidroid.yml
```
Every `with:` block at each caller's default-branch tip:
```
sei-load @ 018776bf (main), uses: uci@68406ee4
seidroid-review mode: review, approve-on-success: true, driver-version: v0.13.0, allowed-team
seidroid-review-close mode: close, driver-version: v0.13.0, allowed-team
seidroid-review-reclaim mode: close, driver-version: v0.13.0
sei-internal-skills @ bbf28e89 (main), uses: uci@c99714ab
seidroid-review mode: review, driver-version: 09ee41de, allowed-team
seidroid-review-close mode: close, driver-version: 09ee41de, allowed-team
seidroid-review-reclaim mode: close, driver-version: 09ee41de
$ grep -n "allow-policies\|allow-tools" <both files at tip>
exit=1 # no match
```
Six sites, none passes `allow-policies` or `allow-tools`. Both pins are
older
than this branch, and both pinned uci revisions do declare the input.
What
matters is what they pass when they bump: nothing. I also read every
historical
version of both files, five commits for sei-load and two for
sei-internal-skills. No version ever passed it. I edited neither caller
repository.
## Verification
```
input count 19 -> 18, PyYAML on workflow_call.inputs; allow-policies absent
YAML parses; jobs guard, review unchanged
placement harness 165 passed, 0 failed, 29 cases;
output byte-identical to the base
actionlint before: 4x SC2102:info after: 4x SC2102:info
same rule set, offsets shifted by the 13 deleted lines
```
`actionlint` exits 1 on both the base and this branch, because an
info-level
shellcheck finding is still a finding. The repository runs no actionlint
in CI,
so this is a local gate.
The base did not move while this was in hand.
`origin/feat/seidroid-review` read
`b1b51f8` at the start and at the push, so this branch needed no rebase.
## What I did not verify
The `seidroid` agent bundle's guardrail config, as stated above. And the
running
deployment: every reading here is source at a named revision, not a live
run.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ment (#102) A superseded thread now closes only once *its own* replacement reached the code. The gate was per review: a review superseding A, B and C closed all three on the strength of whichever replacement happened to place, so a live finding came off a pull request with nothing on the diff where it was. ## What changed **`Place findings on the code`** records each posted comment's superseded thread ids as that comment posts, to `$RUNNER_TEMP/review-superseded-placed.txt`, one id per line. It publishes two outputs: `linkage_path` and `superseded_linked`. **`Resolve the threads this review closed`** closes a superseded thread on finding its id in that record and on nothing else. No count over the review enters the decision, because no count can say which comment replaced which thread. ## The file-comment decision **A comment that degraded to the file rung counts as the replacement.** Four reasons, in the order that decided it: 1. **The finding reached the reader, on the right file.** The file comment sits in the Files-changed view, in the file the author is already in, and its body carries the cited line: `_Cited at `path:line`, outside this diff's changed lines._` 2. **Not closing is the permanent error, closing is the transient one.** The thread being replaced is very often one whose line the current diff no longer covers — that is *why* the comment degraded. Every future re-review will degrade the same way, so the duplicate is not a one-run cost; nothing ever clears it. Closing wrongly costs a thread resolved while its replacement sits one scroll away on the same file. 3. **"On the code" is the invariant; "on the named line" is stronger than the invariant says.** A file-level comment is a review comment on a file in the pull request, in the same conversation list as the thread being closed. 4. **The line the pull request needs it does not have.** The two errors are not symmetric here, and the asymmetry points the other way from the general rule about closing threads. What does **not** count is the summary. A finding in the summary is in the verdict comment — a different object, carrying no thread — so it records nothing and its thread stays open. The 502 path records nothing either: that write may have landed and this run cannot confirm it, so the thread stays open. ## How the linkage survives each placement path | Path | Recorded | |---|---| | the batch posts | every `.anchored[]` finding's ids, in one `jq` write — the call creates all of them or none | | the batch is refused 4xx, the ladder runs | per finding, on the rung that posted | | the batch fails otherwise (5xx, no readable status) | nothing; the write may have landed | | the line rung posts | that finding's ids | | the file rung posts | that finding's ids — the decision above | | either summary group | nothing | | the compare read fails, `base.sha` unreadable, the list is short of `changed_files`, the list is at 300 with no total | `batched=false`, the ladder runs, per finding as above | | the reviewed commit was not recorded | no call can post, so nothing is recorded | The ids ride the per-finding TSV as field 5, ahead of the base64 detail. Only the last field may be empty — `@tsv` writes an empty field as nothing between two tabs and bash folds a run of tabs into one delimiter — so a finding replacing nothing writes a dot, which no node id can be. The order the fields are read in is not the order they are passed in, which keeps `$1`..`$5` where they were. `$RUNNER_TEMP` survives a re-run of a job on a non-ephemeral self-hosted runner, which is the steady state here, so the record is emptied before anything is appended. Case 32b is the test. The record narrows the plan and cannot widen it: an id has to be in `.threads.superseded` **and** in the record. The plan is the driver's warrant; the record is which comment spent it. ## Compatibility `superseded_linked` is `true` when at least one finding in the normalised findings file carries a non-empty `supersedes`, and `false` on every other path including both early exits. The resolve step picks its gate on that one bit: - **published** → per thread. - **absent** → the per-review gate exactly as it stands today, `placed > 0 && ${PLACED_UNPLACED:-1} == 0`. An older driver publishes the key on no finding, so it takes the fallback. A review that supersedes nothing also publishes it on no finding — and then there is no superseded thread for either gate to decide, so the two answers are the same one. No new flag, so the install-time contract check is unchanged. `MIN_DRIVER_VERSION` and the `driver-version` default stay at `v0.15.0`: this file drives a driver older than the linkage without failing, and taking the per-thread gate is a release cut plus a version bump, separately. ## `unplaced == 0` no longer gates the linked path The ticket asked for it to stay as the outer condition. It cannot: the headline acceptance criterion is *"only A's replacement places, A resolves and B and C stay open"*, and in that state `unplaced` is 2. The two requirements contradict each other. The per-thread record subsumes the aggregate. `unplaced` is a count over the whole review; it exists in the fallback precisely because nothing there can name which comment replaced which thread. Where the record can, the count adds nothing and only withholds a correct close. It stays, unchanged, on the fallback path — including the `${PLACED_UNPLACED:-1}` default, which case 39 now pins. ## Preserved - The history read admits `REVIEWER_LOGIN` or `WORKFLOW_LOGIN`; the resolve step keeps the strict single-login test. Cases 40 and 34 hold the split. - Every marker test is `startswith($ENV.FINDING_MARKER)` against the workflow-level `env:`. No `contains` was introduced; case 40 refuses a thread that quotes the marker mid-body. - Both halves still page their GraphQL queries. Case 34 closes a thread that only exists on page two. - `.threads.addressed` closes on publication and needs no record. Case 36. ## Verification The self-check harness at `test/seidroid-review/` was extended, not duplicated. It now extracts **both** steps from the YAML on every run and asserts the two read one `FINDING_MARKER`. `bin/gh` grew a GraphQL arm for the thread read and the resolve mutation. **29 cases / 165 assertions → 47 cases / 271 assertions. Every pre-existing assertion still passes unchanged.** ``` assertions: 271 passed, 0 failed ``` `actionlint` on `seidroid-review.yml` is identical to the base, position-normalised — 4× `SC2102:info`. `shellcheck` on `run.sh` and `bin/gh` is clean. ### The new cases and the mutation that fails each | # | Case | Mutation | Result | |---|---|---|---| | 26 | A places, B and C do not | the linked path closes the whole set once anything landed (W1) | 7 FAIL | | 27 | a replacement degrades to a file comment | the file rung stops recording (W2) | FAIL | | 27b | a replacement on a file whose patch the API did not send | the line rung stops recording (W14) | FAIL | | 28 | the batch is refused, the ladder splits the two threads | the line rung stops recording (W14) | FAIL | | 29 | the compare list is short, the ladder runs | the line rung stops recording (W14) | FAIL | | 30 | an older driver publishes no linkage | `superseded_linked=true` always (W5) | FAIL | | 30b | an older driver whose batch is refused | the dot sentinel is written as an id (W18) | 2 FAIL | | 31 | a 502 records nothing | either summary collector records (W3) | FAIL | | 32 | one comment, six ids, two of them ids | the shape test is dropped from the normalise (W7) | 2 FAIL | | 32b | a second attempt does not inherit the first's record | the `: > "$LINKAGE"` truncation is dropped (W4) | FAIL | | 32c | one comment naming two threads, posted on its own | the recorder writes several ids on one line (W17) | FAIL | | 33 | A closes, B and C hold | per-review gate on the linked path (W1) | 4 FAIL | | 34 | A and C close, one of them on page two | the resolve step iterates the record, not the plan (W8) | 19 FAIL | | 35 | no replacement placed, nothing closes | — contrast with 37 on one bit | | | 36 | `addressed` needs no record | `addressed` is gated on the record too (W13) | 2 FAIL | | 37 | older driver, placement clean, all three close | the fallback never closes (W11); the per-thread gate runs regardless (W20) | 2 / 5 FAIL | | 38 | older driver, one unplaced, none close | the fallback is removed (W11) | FAIL | | 39 | older driver, placings reported, unplaced not | `${PLACED_UNPLACED:-0}` (W12) | 2 FAIL | | 39b | older driver, nothing reported | — | | | 40 | the strict single-login and marker tests | `mine` admits the other identity (W9); `startswith` → `contains` (W10) | 3 / 4 FAIL | | 41 | a thread of ours already resolved | — | | | 42 | the thread read fails | — | | | 43 | the mutation is refused | — | | | 44 | the record cannot widen the plan | the resolve step iterates the record (W8) | 2 FAIL | | 45 | no check file | the guard is removed (W19) | FAIL | | 46 | no `threads` key at all | — | | One mutation also confirms the whole pre-existing suite guards the TSV field order: swapping `supersedes` and the base64 detail in the `tsv` program without swapping the reader fails **90** assertions across 25 cases, `detail` arriving as the severity exactly as that field's own comment warns. ### End to end against real driver output sei-internal-skills#414's `findings.json` and `check.json`, produced by the driver's own code for a reply superseding three threads, fed into both steps extracted from this file: ``` === placement === posted one review carrying 1 comment(s) on owner/repo#7 findings: 1 on a line, 0 on a file, 2 in the summary superseded_linked=true record: PRRT_kwDOABCDEF4Ax1y2 === resolve === review thread PRRT_kwDOABCDEF4Bz3w4 stays open: nothing replacing it reached the code review thread PRRT_kwDOABCDEF4Cq9r8 stays open: nothing replacing it reached the code superseded: 1 of 3 thread(s) had their own replacement reach the code threads: 1 closed, 0 refused, 0 left under another identity, 0 could not be resolved ``` `unplaced` was 2 and A closed anyway, which is the whole change. The same driver output with `supersedes` stripped — byte-for-byte what `v0.16.0` writes for that reply, since no other `Finding` field changed: ``` superseded_linked=false record bytes: 0 3 superseded thread(s) stay open: 1 comment(s) reached the code and 2 could not be placed this review closes no thread ``` and with placement clean, the fallback closes all three. No failure on either. ## Not verified **`resolveReviewThread` itself.** A GraphQL mutation cannot be exercised without a live pull request. Every run above hits a stub that logs the call. What is verified is which thread ids this step decides to call it with, which is where the defect was. The `--paginate` behaviour is likewise stubbed: the stub emits two documents and the step's `jq -s` folds them, which is the contract, but not `gh`'s cursor handling. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… a step that cannot post (#100) A run cancelled by a newer `@seidroid review` now clears the reactions it left on its own trigger comment, from a step that cannot post one. It left the 👀 there for good. Carries **PLT-1166**. **PLT-1159 comes out ruled out**, on the evidence below. Nobody can do it as written. The permission comment records the reason beside the scope the ticket asked to drop. ## PLT-1166 — the defect `Answer the request` took `!cancelled()`. Two `@seidroid review` comments in quick succession put both runs in one concurrency group under `cancel-in-progress`, so the newer one cancels the older. By then the older comment already wears the 👀 — the acknowledgement is the first step of the job. The newer run answers its **own** comment id, so nothing ever reads the older one again. The result: a comment that asked for a review, wears eyes, and never gets an answer. That is the defect PLT-1144 fixed on the no-verdict path, reached by the one path that fix does not cover. ## What ships `Answer the request` keeps `!cancelled()` — unchanged from the base. The withdrawal is a new step, last in the job: ```yaml - name: Withdraw the reactions on a cancelled run if: ${{ inputs.mode == 'review' && cancelled() && needs.guard.outputs.comment_id != '' && steps.verdict.outputs.posted != 'true' }} ``` `Post the verdict` gains `id: verdict` and a `posted` output so that last term can read it, and `Answer the request` gains `id: answer`. Nothing else in the workflow changes. ### Why not `always()` on `Answer the request` That was the first shape here and it was wrong. A step output persists once its step completes, so a cancellation landing any time after `drive` finishes leaves `check_path` and `verdict_produced` populated. `Answer the request` would read a real conclusion and post a thumb, while every publisher skips on `!cancelled()`. A thumb reads as an answer. That is worse than the stale eyes this PR set out to remove. ### Why the new step cannot state an outcome **It contains no POST.** The script lists this bot's reactions and deletes the three this workflow posts. No code path in it adds one. Nothing the step receives can therefore make it state an outcome. That property holds whatever its inputs are, which is what makes it structural rather than a matter of what a cancellation happens to look like. It reads no check file, no `verdict_produced` and no conclusion. ### What the `steps.verdict.outputs.posted` term is, and why it does not break that A cancellation can arrive once the verdict is already on the pull request — during thread resolution, say — and the thumb `Answer the request` posted answers it correctly. Withdrawing it there leaves a published review with no reaction on the request that asked for it, which reads as never answered. That is this step's own defect, one window later. The posting step's **`posted` output** separates the two. It is one boolean about another step, written from the comment POST's own result. No conclusion is in it. It says whether an answer already stands, never which answer it would be, so reading it gives the step nothing to state. Handing it `verdict_produced` instead would have restored the conditional reasoning above: that flag is true whenever the driver reached a verdict, including when nothing published. **Its outcome will not do, and that took a second pass to see.** `Post the verdict` runs under `continue-on-error` and tolerates a refused comment POST. Its failure path ends on a call whose failure it swallows. The step therefore exits 0, and its outcome reads `success` whether the verdict landed or not. The first version of this gate read that outcome. A refused POST followed by a cancellation then kept a thumb standing for a review nobody can see. That step already tracked the POST's result in a shell variable. It now writes it as an output. Anything but a posted verdict withdraws. A value the step cannot read therefore clears. It does not leave a thumb standing for a verdict that may not be on the pull request. ### The cross-run half of the same problem A re-run replays the trigger comment id. The comment can therefore already carry a thumb from an **earlier** run whose verdict is on the pull request. A re-run cancelled before it answered took that thumb along with its own eyes. The comment then ended bare while the verdict it asked for still stood. `Answer the request` gains `id: answer`, and the withdrawal reads its outcome to decide what this run may take: | `steps.answer.outcome` | what it means | withdrawn | |---|---|---| | `skipped` | this run never touched the comment, so a thumb there is an earlier run's | `eyes` only | | `success` | this run withdrew the stale thumb and posted its own, and published nothing | `+1 -1 eyes` | | `failure`, `cancelled`, unreadable | the step ran partway and most likely took the earlier thumb already | `+1 -1 eyes` | The structural property is untouched: still no POST, and an outcome is still four words about another step with no conclusion among them. **One case survives, and the comment states it rather than claiming it away.** A run answers, which withdraws an earlier thumb and posts its own. A cancellation then arrives before publishing, and the comment ends bare. The answer step already took the earlier thumb, so nothing at the end of the job can put it back. Knowing it happened would need a read of the pull request this step deliberately does not make. A cancellation lands during the driver far more often than in that gap. The step's comment records the limit instead of asserting the invariant outright. **One tension with the stated acceptance criterion, deliberately.** "Given a run cancelled by a newer request, its trigger comment carries no reaction from this bot" now fails on one path. A cancelled re-run leaves an earlier run's thumb. The criterion's intent holds. The comment does not wear 👀 with no answer coming, because the answer is on the pull request. Satisfying the literal wording would restore the defect above. I flag it rather than read the criterion loosely. ### Why last in the job, and what holds it there The runner evaluates a step's condition when it reaches the step. **Any step after the withdrawal is a step during which a cancellation leaves the eyes standing.** The runner already evaluated the withdrawal and skipped it by then. Placed last it also reads `steps.verdict.outputs.posted` after that step has reported. `conditions.py` checks the position rather than any one ordering, which covers a step appended later. Four mutations fail it. Move the withdrawal ahead of `Post the verdict`, ahead of the resolve step, or ahead of the no-verdict report. Or append a step after it. The id-ordering check caught only the first. In the other three `Post the verdict` still ran earlier. ### Why not the fix as named Gating the conclusion read inside the script needs the job status in the shell, and GitHub does not offer it there. `cancelled()` is readable only in a step or job `if`. `PipelineTemplateEvaluator.EvaluateStepEnvironment` calls `CreateContext(contextData, expressionFunctions)` with no `expressionState`, where `EvaluateStepIf` passes `step.ExecutionContext.ToExpressionState()`. And `CancelledFunction.EvaluateCore` reads `templateContext.State[nameof(IExecutionContext)]` and `ArgUtil.NotNull`s it. `StepsRunner` turns that throw into `CompleteStep(step, TaskResult.Failed)`, so `env: CANCELLED: ${{ cancelled() }}` fails the step on every run, before the runner evaluates its condition. actionlint refuses it too: `calling function "cancelled" is not allowed here. "cancelled" is only available in "jobs.<job_id>.if", "jobs.<job_id>.steps.if"`. The same holds for `run:`. ## The cancellation shapes, and which this covers | when the cancellation lands | what runs | outcome | covered | |---|---|---|---| | while queued, job never starts | nothing | no eyes were ever posted | n/a | | before `drive` completes | withdrawal | all three withdrawn, no thumb | yes | | **after `drive` completes** | withdrawal | **it cannot read the populated outputs** | yes | | while `Answer the request` runs | its `!cancelled()` re-test fires, the runner kills it, then the withdrawal | the withdrawal takes whatever it left | yes | | after the thumb, before the verdict published | answer, then withdrawal | the withdrawal takes the thumb: it would stand for nothing | yes | | **after the verdict published** | answer only | **thumb survives beside the published verdict** | yes | | **the verdict POST refused, then cancelled** | answer, then withdrawal | **thumb withdrawn: its outcome still reads success** | yes | | during the withdrawal step | withdrawal | its own condition is `cancelled()`, so the re-test keeps it alive | yes | | once the runner reached every step | answer only | a thumb this run earned stays | correct | | a re-run, cancelled before it answered | withdrawal | an earlier run's thumb stays, its eyes go | yes | | a re-run, answered then cancelled before publishing | answer, then withdrawal | bare comment, earlier verdict stands | **no** | | **runner process shutdown** (`RunnerShutdownToken`) | nothing | `StepsRunner` skips condition evaluation outright | **no** | Two rows are gaps. In the answered-then-cancelled row the answer step has already taken the thumb, so no later step can restore it. A hard kill of the runner leaves the eyes on the comment, and nothing inside a workflow closes that. ## The reaction table Each case declares two job states: the one the runner reached `Answer the request` in, and the one it reached the withdrawal step in. `success>cancelled` is a cancellation that arrived after the answer, so the answer step posts its own thumb and the fixture places nothing by hand. | case | states | verdict outcome | ran | left on the comment | |---|---|---|---|---| | success | `success>success` | success | answer | `bot:+1` | | failure | `success>success` | success | answer | `bot:-1` | | no verdict | `success>success` | skipped | answer | *none* | | neutral | `success>success` | skipped | answer | *none* | | **cancelled after `drive`, outputs populated** | `cancelled>cancelled` | skipped | withdraw | ***none*** | | cancelled before `drive` finished | `cancelled>cancelled` | skipped | withdraw | *none* | | cancelled mid-publish | `success>cancelled` | cancelled | answer+withdraw | *none* | | the verdict failed to post | `success>cancelled` | failure | answer+withdraw | *none* | | the outcome went unreported | `success>cancelled` | *empty* | answer+withdraw | *none* | | **cancelled after the verdict published** | `success>cancelled` | success | answer | ***`bot:+1`*** | | the same, beside a human's | `success>cancelled` | success | answer | `brandon:-1`, `bot:+1` | | success, human `+1 -1 eyes` | `success>success` | success | answer | human ×3, `bot:+1` | | failure, human ×3 | `success>success` | success | answer | human ×3, `bot:-1` | | no verdict, human ×3 | `success>success` | skipped | answer | human ×3 | | cancelled, human ×3 | `cancelled>cancelled` | skipped | withdraw | human ×3 | | stale `bot:-1` + `human:+1` | `success>success` | success | answer | `human:+1`, `bot:+1` | | the same, cancelled | `cancelled>cancelled` | skipped | withdraw | `human:+1` | | stale `bot:+1`, no verdict | `success>success` | skipped | answer | *none* | | `bot:rocket` from another workflow | `success>success` | success | answer | `human:+1`, `bot:+1`, `bot:rocket` | | the same, cancelled | `cancelled>cancelled` | skipped | withdraw | `human:+1`, `bot:rocket` | | the list call refused | `success>success` | success | answer | `human:+1`, `bot:+1`, `bot:eyes` + warning | | a delete refused | `success>success` | success | answer | `bot:+1`, `bot:eyes` + warning | | the add refused | `success>success` | success | answer | *none* + warning | | the list refused, cancelled | `cancelled>cancelled` | skipped | withdraw | `bot:eyes` + warning | | a delete refused, cancelled | `cancelled>cancelled` | skipped | withdraw | `bot:eyes` + warning | | the acknowledgement refused | `success>success` | success | answer | `bot:+1` | A human's reaction survives every path. A `bot:rocket` some other workflow left survives too, because each step deletes only the contents this workflow posts. And a thumb that answers a published verdict survives a later cancellation. ## PLT-1159 — ruled out, with the evidence The ticket's premise is that `ai-review.yml` reaches the same reactions through GraphQL `addReaction` under `pull-requests: write`. **It does not, and the workflow file does not decide the question.** **1. `ai-review.yml` does call both mutations, and its `permissions:` blocks do lack `issues`.** `preflight` is `contents: read` + `pull-requests: write` and calls `addReaction(content: EYES)`; `complete_review_reaction` is `pull-requests: write` alone and calls `addReaction(THUMBS_UP)` then `removeReaction(EYES)`. **2. But neither call uses `GITHUB_TOKEN`.** Both steps pass `github-token: ${{ steps.app-token.outputs.token || github.token }}`, and in production the App token wins. I checked live comments. Every reaction on an `@seidroid review` trigger in `sei-chain` belongs to `seidroid[bot]`, not to `github-actions[bot]`: ``` comment 5536974191 +1 by seidroid[bot] sei-chain#4088 comment 5544795940 +1 by seidroid[bot] sei-chain#4101 comment 5531689281 +1 by seidroid[bot] sei-chain#4095 comment 5493838638 +1 by seidroid[bot] sei-chain#4063 ``` The workflow's `permissions:` block does not bound an App installation token. Its own installation grant governs, and that App holds Issues: write — `ai-assistant.yml` posts `POST /repos/{o}/{r}/issues/comments/{id}/reactions` with the same token. ai-review is therefore **no evidence at all** about what `pull-requests: write` alone can do. It is the same class of wrong premise as the `enable-cursor: false` one. **3. GitHub documents no permission for any GraphQL mutation.** Not a gap in my reading — checked at the data source. In `github/docs`, `src/graphql/data/fpt/schema-reactions.json` gives `addReaction` and `removeReaction` the keys `name, id, href, description, isDeprecated, inputFields, returnFields, category` and no permission field. The public SDL (`docs.github.com/public/fpt/schema.docs.graphql`) carries only `@docsCategory(name: "reactions")`. The GraphQL guide's whole statement on the subject is that the API returns an error naming the permission it wanted. One route therefore remains to the requirement: make the call. **4. GitHub documents what REST requires, and the alias stops at the reaction.** `github/docs`, `src/github-apps/data/fpt-2026-03-10/server-to-server-permissions.json`: | endpoint | permission | |---|---| | `GET/PATCH/DELETE /repos/{o}/{r}/issues/comments/{id}` | listed under **both** `issues` and `pull_requests` | | `POST /repos/{o}/{r}/issues/comments/{id}/reactions` | `issues: write` **only** | | `DELETE /repos/{o}/{r}/issues/comments/{id}/reactions/{rid}` | `issues: write` **only** | | `POST /repos/{o}/{r}/pulls/comments/{id}/reactions` | `pull_requests: write` (a *review* comment — a different resource) | That file expresses "either permission" by listing an endpoint twice. A single listing on the reactions endpoints is therefore a distinction, not an omission. It confirms the claim the guard job already makes in prose. **Verdict.** The premise is void and the documentation says nothing. Nothing here can mint a fine-grained token scoped to `pull-requests` to test it. Shipping the drop blind would regress the defect this PR fixes. `continue-on-error` and a `::warning::` swallow a 403 on the reaction, so the eyes would stay on every comment and no run would fail. Ruled out. ### Two things worth keeping for whoever re-files it **`removeReaction` beats the REST loop, whatever the scope turns out to be.** The ticket assumed it takes a reaction node id. It does not. `RemoveReactionInput` is `{content: ReactionContent!, subjectId: ID!}`, and the subject is the *comment*: `IssueComment` sits in its `@possibleTypes`. It takes no actor input, so it can only ever remove the viewer's own reaction. That makes the human-scoping property structural rather than a `select(.user.login == $me)` filter. It also retires the hardcoded `me="github-actions[bot]"` login and the paginated list whose miss leaves eyes behind. **A third shape the ticket does not name looks likelier than either.** This workflow already mints an App token (`steps.identity.outputs.token`) and already computes `REVIEWER_LOGIN` from `app-slug`. Reacting under that identity needs no caller scope at all, and it is how production already posts these reactions. Two obstacles stand in the way. The acknowledgement runs before the mint, on purpose. And the mint is optional, so a `GITHUB_TOKEN` fallback keeps the scope required — unless a caller without App credentials may lose the reaction. ## The cost this change carries The list-and-delete block is now duplicated between `Answer the request` and the withdrawal step. `me="github-actions[bot]"` and the set of contents each step may delete are two copies, and a reader has to keep them in step by hand. Edit one and not the other and a reaction stays behind on whichever path lost the edit. That is the price of the separate step, and it buys the structural property: the withdrawing step has no POST. Sharing the block would mean one step doing both jobs, which is the shape that produced this PR's blocker. GitHub Actions offers no way to share a script between two steps without a checkout, and YAML anchors are not supported. Both harnesses cover both copies, so a drift fails rather than ships. The GraphQL `removeReaction` above is what would remove the duplication outright. It needs no login and no listing, so the whole block collapses to one mutation per content. ## Every reaction site Three steps, six calls, all in the `review` job, all on the ISSUE comments endpoint: | line | step | call | |---|---|---| | 1036 | `Acknowledge the trigger` | `POST .../issues/comments/{id}/reactions` (`eyes`) | | 2494 | `Answer the request` | `GET .../reactions --paginate` | | 2504 | `Answer the request` | `DELETE .../reactions/{rid}` | | 2519 | `Answer the request` | `POST .../reactions` (`+1` / `-1`) | | 3292 | `Withdraw the reactions on a cancelled run` | `GET .../reactions --paginate` | | 3304 | `Withdraw the reactions on a cancelled run` | `DELETE .../reactions/{rid}` | The withdrawal step has no `POST`, and that is the fix. `guard` reacts nowhere. `ai-assistant.yml` and `ai-review.yml` have their own sites; neither is in this workflow. ## Verification **Committed, not kept locally.** An uncommitted harness is how the first blocker survived a reading and seven mutations. Two additions to `test/seidroid-review/`, wired into `workflow-test-self.yml` as their own job so the placement check keeps its name. **`reactions.sh` — 62 assertions over 32 cases.** It runs the reaction steps under `bash`, extracted from the workflow on every run. No case names the step it runs. `conditions.py --select` names it, from the job state and the posting step's outcome, so the two layers cannot drift. The `gh` stub keeps the reaction list a comment carries and serves it through the step's own `--jq`. It honours idempotence per (user, content). It can refuse the list, a delete or the add. The acknowledgement's calls go to a separate log, so every count belongs to the step under test. The stub reports any call it cannot serve. **`conditions.py` — 76 assertions.** A step condition decides which reaction step runs in which job state, and a shell harness cannot see it. The model applies the runner's own rule: a condition naming none of `always`/`cancelled`/`failure`/`success` becomes `success() && (...)`. It treats a term it cannot decide as unknown rather than false. Two checks read the file rather than a table, so they cover a step added later: Both walk **every job's raw steps list** and search the **whole step**: - No step that can run on a cancelled job may reach `check_path` or `verdict_produced`. - Every `steps.<id>` a step reads must be a real id on an earlier step. - The withdrawal is the last step of the review job. Keyed off a name they dropped an unnamed step. `- uses: ...` with no `name:` is the usual shape, so the step most likely to arrive later was the one they could not see. The guard job already carries one. The id check also reached only `if`, which left the withdrawal's new `env` read unchecked. **Each fixture, mutation-tested.** A fixture that cannot fail the invariant is not a test of it: | mutation | which case fails | |---|---| | **the gate reads `steps.verdict.outcome` again** | **`THUMB GOES` on a refused POST, and the matching condition row** | | **the `skipped` arm takes all three** | **`EARLIER THUMB SURVIVES`, plus 3 more** | | **`id: answer` deleted** | **`reads steps.answer, which is no step's id`** | | **an unnamed `always()` step reading `verdict_produced`** | **`step 17 runs on a cancelled run and reads verdict_produced`** | | the `posted` term dropped | `THUMB SURVIVES` + 3 more + the condition row | | the same term inverted | 16 script cases, 5 condition cases | | `id: verdict` deleted | `reads steps.verdict, which is no step's id` | | the withdrawal moved ahead of `Post the verdict` | the position check, and `reads steps.verdict, which runs later` | | **the withdrawal moved ahead of the resolve step** | **the position check alone — the id check passes it** | | **the withdrawal moved ahead of the no-verdict report** | **the position check alone** | | **any step appended after the withdrawal** | **the position check** | | `Answer the request` back to `always()` | its cancelled row, and the sweep | | `check_path` interpolated inline into `run:` | the sweep — an `env`-only sweep passes it | | the withdrawal step deleted | `no step named 'Withdraw the reactions on a cancelled run'` | | the withdrawal not scoped to this bot | the human and `rocket` cancelled cases | | the withdrawal ignoring which contents it may take | `foreign-cancelled` loses a `rocket` | | the missing-conclusion arm clearing only the eyes | cancelled-with-stale-thumb, no-verdict-with-stale-thumb | | the answer's list not scoped to this bot | every human case | | the empty-reaction guard removed | every clear-only case posts a blank reaction | | the `VERDICT_PRODUCED` gate removed | no verdict thumbs the requester down | | the success arm no longer naming the eyes | eyes survive a green review | Only `conditions.py` catches `id: answer` deleted. `reactions.sh` derives that outcome itself, which is exactly why the id check has to exist. `conditions.py` also models `steps.verdict.outcome` beside `posted`, derived rather than passed. It reads `success` whenever that step reported at all, which is what the runner sees. A gate that regresses to the outcome therefore fails an assertion instead of crashing the model. **Five checks ran narrower than their own claim.** Mutating the thing each claimed to cover is what found them. - The haystack read `env` only, so an inline interpolation passed. - The model took an outcome as an argument and never checked the id existed. - The sweep keyed off a step name, so an unnamed step was invisible. - `run_case` exported each per-case knob without clearing it. An `ANSWERED_AS` override leaked forward and disarmed a later case. - The gate read an exit code that cannot express whether the verdict landed. The first three now read the file. `run_case` clears the fourth at the top of every case. The fifth reads an output written from the POST's own result. **actionlint**, base vs branch, both rule sets. `seidroid-review.yml`: 4 × `SC2102:info`, unchanged. Whole `.github/workflows`: 37 findings, identical after line-number normalisation. `workflow-test-self.yml`: 0. `shellcheck -S warning` clean on both new harness files. The incumbent harness still passes, now 271 assertions after #102. **Base checked three times, and no move trusted.** It went to `b1b51f8` (#97), then `2f58b80` (#101), then `98c2619` (#102). #97 rewrote 341 lines of this file and added the harness. #102 rewrote the resolve step and placement, and renamed the harness job. Each time all three harnesses and actionlint ran again on the new history. None of them carried a result from before it. I checked each rebase by reading back four things: the step order, the step ids, the gate, and the withdrawal step's POST count. Not by its exit status. The #102 rebase conflicted once, in the harness README, and I resolved it keeping both sides. `workflow-test-self.yml` merged cleanly: `place-findings` keeps #102's renamed display name `Place findings and resolve threads`, and the reaction job keeps its own. **One handoff for whoever lands second.** #103 replaces the hardcoded `issues/comments` with a `comment_api` output. Its author wrote it against **two** reaction steps. This branch leaves three, carrying six hardcoded paths rather than four: one in `Acknowledge the trigger`, three in `Answer the request`, two in the withdrawal step. Whichever of us rebases second has to reach all three steps. ## Not verified **Nothing here ran on a GitHub runner.** Five things stay unverified. That a real cancelled run reaches the last step of the job. That `steps.verdict.outcome` reads `success` on a run cancelled after that step completed. That `steps.answer.outcome` reads `skipped` rather than empty on a run cancelled before that step. That the runner's condition re-test kills `Answer the request` mid-flight, as its source says. And every permission claim above. The cancellation semantics rest on `actions/runner` source and on actionlint. `conditions.py` models the expression engine; it is not the engine. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…let a caller name it (#103) Four tickets, one region: the guard's job condition, its `parse` and `Admit the request` steps, and the two `workflow_call` inputs they read. **PLT-1147 — accept `pull_request_review_comment` and `pull_request_review`.** Both events are admitted. Every read in `parse` and `Admit the request` takes the comment key or the review key, whichever the event populated: a review body names its author under `review.user` and its id under `review.id`, and a step reading `comment.*` alone saw an empty body there and refused in silence. The two diff-side events are held to their creating action, because a `dismissed` review replays the body of the review it dismisses — a caller wiring that type would re-review on every dismissal. **PLT-1153 — restore `allowed-bots`.** A JSON array of exact logins, default `[]`. Checked in the guard's job condition, so an unlisted bot starts no runner, and again in `Admit the request`. Exact and case-insensitive both times, as ai-review.yml checks it. A listed bot skips the team read — a bot is not a team member — and is held to the fork check, the skip label and the command grammar. The job condition now admits a person on `author_association` and a bot only by login, because association does not discriminate a bot: one with write access carries MEMBER like anyone else. **PLT-1161 — refuse an unsupported event.** A first step names the event that arrived and the four this workflow handles, and exits 1. It runs before the identity mint and before the secret check, so a mis-wired caller spends no credential. The guard's condition gained a clause admitting an unsupported event for exactly that step: without it the job is skipped, every job after it is skipped, and the run reports success having done nothing. `pull_request` is excluded from that clause — a `pull_request` close skips the guard deliberately, and the review job reads that skip as its own trigger. `pull_request_target` is refused apart, with its reason: it runs with the base repository's secrets and a writable token over a head this workflow did not check out. Nothing here checks anything out today; the refusal is the control that does not depend on that staying true. **PLT-1164 — restore `trigger-phrase`.** Default `@seidroid`, and the pattern is built from it rather than hardcoded — in the command grammar and in the repository-target refusal beside it. ## The two decisions the tickets asked for **The optional `@` stays.** A person who types the phrase without the mention still means it, and the wider form costs nothing here. Whole-line anchoring is what makes it safe, and it is intact. The non-overlap with `ai-assistant.yml` is now measured rather than argued: that workflow's reply condition requires `contains(body, '@seidroid')`, so a bare `seidroid review` reaches this workflow alone. Group 16 of the harness evaluates the assistant's own condition beside the parse for five bodies and records which tool answers each. Two bodies both tools answer today, and both predate this change: `@seidroid review close`, and a body carrying the command on its own line amid prose. The assistant reserves the exact body only, and neither of those is it. Whole-line anchoring is what admits the second — and it is also what keeps `Do we need @seidroid review here?` from starting a review, so the overlap is the price of the property the ticket told me not to lose. The harness asserts the present, so a later change that closes either overlap fails a case and has to re-read it. **The phrase's shape is constrained, not escaped.** After stripping one leading `@`, the phrase must be letters, digits, `_` and `-`. None of those is an ERE metacharacter, so the pattern carries the phrase verbatim with no escaping. Anything else falls back to `@seidroid` with a warning, which is what `guidelines-file` does with a name it cannot trust. Escaping would have to cover every ERE metacharacter correctly forever; a character class is one thing to read. Two harness cases show what the constraint buys: with `@my.bot`, `@myXbot review` does not match; with `@a|b`, the line `a note about the diff` does not match. Unconstrained, the `|` would split the pattern into `^[[:space:]]*@?a` — which every line starting with `a` matches. ## One deliberate step outside the stated region Three reaction steps build their reactions URL from a new guard output, `comment_api`, instead of a hardcoded `issues/comments`: `Acknowledge the trigger` (one path), `Answer the request` (three) and, since #100, `Withdraw the reactions on a cancelled run` (two). Six paths, three `env:` keys. The endpoint differs per event — `issues/comments/{id}/reactions` for a conversation comment, `pulls/comments/{id}/reactions` for a diff-thread one — and without this PLT-1147's acknowledgement would post to a path that holds no object, and the two steps that withdraw it would look for it somewhere else again. That is the trap the ticket names, and it cannot be fixed from inside the guard alone. The review job already holds both scopes: GitHub grants the first to Issues and the second to Pull requests. Two properties of #100 survive the edit, and both are asserted rather than argued. The withdrawal step is still the **last** step of the review job — index 16 of 17, and `conditions.py` checks the position rather than one ordering. And it still contains **zero POSTs**: `-X POST`, `--method POST` and `-f content` each appear 0 times in it, `DELETE` is the only verb it names, and `reactions.sh` asserts the POST count. Adding an `env:` key changes neither. `repos/{owner}/{repo}/issues/comments/{id}` in two other steps is untouched: those delete comments this workflow posted on the conversation, not the trigger. ## One acceptance criterion that REST cannot meet GitHub publishes no reactions endpoint for a pull request **review**. Only the GraphQL schema makes a review reactable, and PLT-1159 already proposed that route and was declined. So a command in a review body starts a review, `comment_api` and `comment_id` both go out empty, both reacting steps skip on their existing condition, and a `::notice::` in the run log says the review started and why no reaction landed. The review, the verdict comment and the inline findings all still arrive. A diff-thread comment gets the full acknowledgement. That is a read claim, not a measured one — see below. ## The review round Seven findings taken. **An unset `allowed-bots` no longer takes the run down.** A `workflow_call` default applies only to an input the caller OMITS, so `allowed-bots: ${{ vars.SOMETHING }}` with that variable unset arrives as `''`, and `fromJSON('')` is not `[]`. The condition reads `fromJSON(inputs.allowed-bots || '[]')`, so empty takes the documented default and denies every bot, while a non-empty non-JSON value still fails loudly. The fix holds under either evaluation order, which turned out to matter — see below. **`gha.py` short-circuits, because the runner does.** Or and And return on the first truthy or falsy operand and never evaluate the rest. My model evaluated eagerly, and one shipped assertion therefore stated the opposite of what a real event does: with a malformed list and a human MEMBER, the person branch is already true, so `fromJSON` is never reached and the guard admits. Re-derived per requester — a person yields `true`, a bot yields `error` (the requester whose admission depends on parsing the list), an automatic review yields `true`. This corrects a claim in my own earlier report, where I had listed eager evaluation as read-not-measured and had it backwards. **Group 16 now measures the overlap on all three events.** `claims()` was keyed to `issue_comment`, so it measured the division of labour on the one path that already had it and inferred the two this branch adds. It takes an event now, and every body runs on all three plus an empty review body. The overlap is identical on all three — measured, not reasoned. 11 assertions to 33. **`ai-assistant.yml` is in `workflow-test-self.yml`'s `paths:`.** Group 16 states an invariant about that file, so an edit there could break it and surface later as a red `Guard the request` on an unrelated change. I audited every file the three harnesses read: it was the only one outside the filter, and `conditions.py` takes its target from the CI command line, which names a watched file. No other cross-file assertion has this shape. **The log id and the reactable id are two facts.** `comment_id` is the reactable object and goes out empty where nothing can react; the driver's `--trigger-id` was reading it, so a review-body dispatch had silently stopped carrying a label. The guard emits `trigger_id` beside it, always populated, and only `Drive session + collect verdict` moved to it — no step condition changed, so `conditions.py`'s context model needed nothing. A new group asserts which of the four outputs each consumer reads. **A comment claimed something false about the payload.** "No comment event carries a head repository" holds for `issue_comment` alone; `pull_request_review_comment` and `pull_request_review` both carry `pull_request.head.repo.id` and `.base.repo.id`. I corrected the sentence rather than widening the branch, because the label check twelve lines below reads the same `GET /repos/{owner}/{repo}/pulls/{n}` endpoint unconditionally on all three comment paths: reading the payload here would drop one of two identical round trips and neither the failure mode nor the dependency. The comment now names the one event that needs the API and records what a payload-keyed branch would have to preserve. The bigger saving is collapsing those two reads of one endpoint into one, available on all four paths — that belongs in a ticket, because it moves the fork check. **The permissions comment explains both scopes**, one per collection, and names what pruning either costs: the reaction fails on the path that scope serves, and all three reacting steps treat a lost reaction as a courtesy and only warn. ## Verification Everything below ran on this machine. Nothing ran on a GitHub runner. `test/seidroid-review/run-guard.sh` is new, beside the placement harness. It reads five steps out of the shipped YAML by name or id, runs them under `bash` against a `gh` stub of its own, and evaluates the two job conditions, the per-event `env:` mappings and the declared input defaults with a new `gha.py`. ``` $ test/seidroid-review/run-guard.sh assertions: 241 passed, 0 failed $ test/seidroid-review/run.sh # placement and resolution, unchanged assertions: 271 passed, 0 failed $ test/seidroid-review/reactions.sh # 62 before, +15 for the collection assertions: 77 passed, 0 failed $ python3 test/seidroid-review/conditions.py .github/workflows/seidroid-review.yml assertions: 77 passed, 0 failed ``` `reactions.sh` needed the change, not just the extra cases. Its `run_case` did not export `COMMENT_API`, so every extracted step died on an unset variable under `set -u` and 25 of its 62 assertions failed with every API count at zero. The default is set there now, and a group varies it: three steps, six paths, and a `pulls/comments` case asserting nothing reached `issues/comments`. `conditions.py` went from 76 to 77 on its own. Two of its checks walk every job's raw steps list, so the refusal step this branch adds to the guard job earns one more assertion without anything being written for it. `gha.py` models four GitHub expression semantics the conditions rest on: case-insensitive string comparison, `||` and `&&` yielding one operand each, **both short-circuiting**, and `contains` over an array testing membership rather than substring. `--selftest` checks all eighteen readings, and group 0 of the run fails if any is wrong. The model is read from GitHub's published semantics; it is not measured against a runner. **Mutation check.** 41 mutations of the shipped workflow, applied one at a time, each killed at least one assertion. **0 alive, 0 skipped.** The sweep runs all four harnesses per mutation, because one edit spans steps three of them cover — a mutation only `reactions.sh` or `conditions.py` can see would have survived a sweep that ran the guard harness alone. Four of the 41 cover this review round: dropping the empty-input fallback, holding `trigger_id` back with the reactable id, and pointing either the driver or the acknowledgement at the other's id. Among them: dropping either new event from the condition, dropping the creating-action gates, reading `allowed-bots` as a string rather than JSON, dropping the `pull_request_target` arm, emitting the id where no endpoint reaches it, dropping the phrase's shape check, hardcoding the phrase in either pattern, dropping the whole-line anchors, requiring the `@`, reading only the `comment.*` payload keys in either step, matching a listed bot by substring or case-sensitively, letting the once-per-PR gate reach a comment, and applying the requester check to a teardown. Six mutations cover the six reaction paths — one in the acknowledgement, three in the answer, two in the withdrawal — and each is killed by at least two assertions. Two more cover #100's properties: appending a step after the withdrawal is killed by `conditions.py`'s position check, and adding a POST to the withdrawal step is killed 21 times by `reactions.sh`. **actionlint, before and after.** Base `2f7efad`, all workflows: ``` 6 [action] 30 [shellcheck] 1 [syntax-check] ``` This branch, all workflows: the identical set, finding for finding — compared as `rule + code`, not just as a count, and identical per file too. `seidroid-review.yml`'s own four are the pre-existing `SC2102:info`. The rest are in `ai-assistant.yml` (3), `ai-review.yml` (4), `release-check.yml` (22) and `release-publish.yml` (4), all untouched. `workflow-test-self.yml` lints clean. Both files parse under PyYAML. ## Rebase note Written against `3544bf5`; rebased three times as the base moved, to `b1b51f8` (#97), `98c2619` (#101, #102) and `2f7efad` (#100). Head is `a31efa6`. The third rebase conflicted in four files: - **`seidroid-review.yml`** — one hunk, in `Acknowledge the trigger`: #100 rewrote the comment above the POST while this branch rewrote the URL below it. Union. - **`workflow-test-self.yml`** — a three-way union. Three jobs now, under distinct names: `Place findings and resolve threads`, `The reaction steps`, `Guard the request`. - **`README.md`** — two hunks; one document with a section per harness. - **`.gitignore`** — union of three extractor lists. One collision the conflict markers did not show: `reactions.sh` and `run-guard.sh` both extract `Acknowledge the trigger` and `Answer the request`, and both wrote them to `ack.sh` and `answer.sh`. Running both would have one overwrite the other's extraction. This branch is the newcomer, so it moved: `guard-ack.sh` and `guard-answer.sh`. The README now says which harness asks what of those two steps. Everything was re-run on the rebased history rather than carried forward: all four harnesses, the full mutation sweep, the actionlint comparison against the new base, and the group counts, recounted from the shipped file (unchanged this time — 204 over the same seventeen groups). **The sweep caught itself.** Its first pass on this base reported 36 killed and **one SKIP**: `M28`, which hardcodes the issue collection in the answering step's read. After this branch routed the withdrawal step through `COMMENT_API`, that step's read became byte-identical to the answering step's, so `M28`'s anchor matched twice and stopped applying. A sweep that only counted kills would have read 36/36 and looked clean. `M28` now carries the comment line above the call, which the two steps do not share, and is killed by four assertions across two harnesses. ## What rests on reading rather than measurement - That GitHub sends `pull_request_review_comment` as `created` and `pull_request_review` as `submitted` for a new request, and that a dismissal replays the dismissed review's body. - That the REST API carries no reactions endpoint for a pull request review. - That `fromJSON` over a non-JSON input fails the expression rather than evaluating false, and that GitHub evaluates both operands of `||`. - Every semantic `gha.py` models. A case here can only be as right as that model. - That a step with an explicit `if:` still requires the steps before it to have succeeded, which is what makes the refusal skip the identity mint. The file's own comments already rest on this. Nothing in this branch has been exercised by a real event on a runner. ## Case table `Guard the request`: 241 assertions — 95 runs of an extracted step script, and 71 call sites evaluating a shipped condition, `env:` mapping or declared input. Recounted from the shipped file. | Group | Assertions | What it holds | |---|---|---| | 0 | 1 | the expression model `gha.py` uses | | 1 | 11 | which requests reach a runner, on all three comment events | | 2 | 14 | `allowed-bots` in the job condition, malformed and unset | | 3 | 5 | the events the workflow does not handle | | 4 | 8 | the review job's condition | | 5 | 17 | the refusal, by event | | 6 | 28 | the parse: which body is a command, and what it resolves to | | 7 | 18 | a caller's own trigger phrase, including regex metacharacters | | 8 | 20 | who may ask, on every comment path | | 9 | 20 | a bot held to `allowed-bots` | | 10 | 17 | fork, label and once-per-PR, on the new paths | | 11 | 11 | draft, first review, re-review and teardown | | 12 | 16 | the payload field each step reads, per event | | 12b | 5 | which guard output each consumer reads | | 13 | 5 | the defaults a caller inherits | | 14 | 5 | the acknowledgement's collection | | 15 | 7 | the answer's collection | | 16 | 33 | what `ai-assistant.yml` claims of the same body, on each event | `The reaction steps`: 77 + 77. `reactions.sh` carries 15 assertions over four cases for the collection each of the three steps reaches; `conditions.py` gains 1 for the guard's new refusal step, which its file-wide sweep picks up on its own. `Place findings and resolve threads`: 271, untouched. PLT-1147 PLT-1153 PLT-1161 PLT-1164 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two literals moved together, as the floor's own comment requires: `driver-version`'s default and `MIN_DRIVER_VERSION`, both `v0.15.0` → `v0.17.0`. A caller may run ahead of the default, never behind it. ## Why now **`v0.17.0` publishes `supersedes` per finding** — the prior threads that finding replaces. The resolve step reads it to close each superseded thread only once *that finding's own* replacement reached the code. On `v0.15.0` or `v0.16.0` the field is absent, so the step reports `superseded_linked=false` and falls back to its per-review gate: a review superseding threads A, B and C closes all three on the strength of one unrelated finding placing. The per-thread gate shipped in #102 is inert until this lands. The floor also crosses **`v0.16.0`**, which bounds the prior-thread history by bytes rather than by a count of twenty threads and three replies — the same budget that carries 645 one-line findings where the count carried 20. ## Also updated The `MIN_DRIVER_VERSION` comment's version ladder, which contrasts what each release concludes or carries. It now runs to `v0.17.0`. The two remaining `v0.15.0` mentions are both in that ladder, as contrasts, and stay true. Both `go install` examples in the prose now name `v0.17.0`. ## What this refuses Any caller pinning below `v0.17.0` fails at install with a named message rather than mid-review. Both callers pin `uses:` by sha and still run an older workflow, so nothing breaks today — but each cutover must drop its `driver-version` line **in the same commit** that bumps its `uses:` sha. Recorded on PLT-1165, PLT-1170 and PLT-1174. ## Verification ``` go install …@v0.17.0, cold GOMODCACHE resolves, mod version v0.17.0 (go: downloading … proves the cache was empty) Supersedes in the tag findings.go:59, both tag forms actionlint base 4 SC2102 actionlint head 4 SC2102 identical yaml.safe_load parses run.sh 271 passed, 0 failed run-guard.sh 241 passed, 0 failed reactions.sh 77 passed, 0 failed conditions.py 77 passed, 0 failed ``` Not verified: nothing ran on a GitHub runner. The install step's floor comparison was exercised against real `go install`s when it shipped; this change moves its constant and does not touch its logic. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The runner pins no model, so it launches on whatever the Claude CLI reports as its own default. That is claude-opus-4-8[1m] today, and it moves on a base image rebuild with no change in this repository. Measured on the deployed server before choosing the value: a session launched with --model claude-opus-5 reports llm_model=claude-opus-5 and answers its first turn, so the runner's credential serves the model. Smart Routing is off, no agent spec names a model, and the secret holds no model variable, so this input is the only lever in the path. The value is unvalidated by design and fails at turn start when wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous commit set the default to claude-opus-5, unsuffixed. The default it replaced was claude-opus-4-8[1m], so that quietly narrowed every review from the 1M window to the standard one. A long diff would compact sooner and the reviewer would read less of it, with no error to say so. seidroid caught this on platform#1651. Measured before choosing the value: a session launched with --model 'claude-opus-5[1m]' reports llm_model=claude-opus-5[1m] and answers its first turn. The earlier probes used the unsuffixed id, so servability of the suffixed one was not established until now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
v0.18.0 (sei-protocol/sei-internal-skills) ships two transport fixes traced to a recurring ~20-29% no-verdict crash rate across every caller of this workflow: mint the machine-credential token on the health-checked transport instead of a bare client with no dead-connection detection, and retry a session lookup that never reached the server instead of failing the run outright. Moves both halves together per this file's own note: the driver-version default and MIN_DRIVER_VERSION are one value in two places, and only raising both keeps a caller that omits the input (all five today) actually running the fixed driver rather than being admitted by a floor the default no longer matches. Branched from the exact commit every current caller pins (d895e04), not from main: this workflow file has since moved on main in ways the callers' pinned SHA does not track, and reconciling that is its own, separate change. This commit exists to be referenced by its own SHA from each caller's `uses:` line, the same way they already reference the commit it descends from -- not to be merged into main as-is. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
PR SummaryLow Risk Overview The bump updates Why: v0.18.0 of Reviewed by Cursor Bugbot for commit 92f74c9. Bugbot is set up for automated code reviews on this repo. Configure here. |
This was referenced Sep 10, 2026
bdchatham
added a commit
to sei-protocol/sei-internal-skills
that referenced
this pull request
Sep 10, 2026
Bumps this repo's three `seidroid-review.yml` references from `sei-protocol/uci@d895e04` to `@92f74c9` -- the commit that raises the reusable workflow's driver floor from `v0.17.0` to `v0.18.0`. v0.18.0 (sei-internal-skills) ships two transport fixes traced to a recurring ~20-29% no-verdict crash rate on seidroid-review across all five repos that use this workflow: mint the machine-credential token on the health-checked transport instead of a bare client with no dead-connection detection, and retry a session lookup that never reached the server instead of failing the run outright. Depends on sei-protocol/uci#106 landing first (or at minimum, this SHA staying reachable in that repo -- it doesn't require the uci PR to merge into `uci`'s `main`, only that the commit exists). Not merging this myself -- opening for review. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
bdchatham
added a commit
to sei-protocol/sei-k8s-controller
that referenced
this pull request
Sep 10, 2026
Points at sei-protocol/uci's seidroid-review.yml at the commit that raises the driver floor to v0.18.0 (sei-protocol/sei-internal-skills), carrying two transport fixes traced to a recurring no-verdict crash rate on this workflow: mint the machine-credential token on the health-checked transport (uci#420-equivalent upstream fix), and retry a session lookup that never reached the server instead of failing the run outright. See sei-protocol/uci#106 for the full change this SHA carries. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
bdchatham
added a commit
to sei-protocol/seictl
that referenced
this pull request
Sep 10, 2026
Bumps this repo's three `seidroid-review.yml` references from `sei-protocol/uci@d895e04` to `@92f74c9` -- the commit that raises the reusable workflow's driver floor from `v0.17.0` to `v0.18.0`. v0.18.0 (sei-internal-skills) ships two transport fixes traced to a recurring ~20-29% no-verdict crash rate on seidroid-review across all five repos that use this workflow: mint the machine-credential token on the health-checked transport instead of a bare client with no dead-connection detection, and retry a session lookup that never reached the server instead of failing the run outright. Depends on sei-protocol/uci#106 landing first (or at minimum, this SHA staying reachable in that repo -- it doesn't require the uci PR to merge into `uci`'s `main`, only that the commit exists). Not merging this myself -- opening for review. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The intended payload — raising the sei-agent-driver floor from v0.17.0 to v0.18.0 — is applied completely and consistently (input default, MIN_DRIVER_VERSION, and both doc examples), but the PR as filed against main is not a four-line bump: it adds 6,885 lines across 43 files that main does not carry, so merging it as-is lands a second, parallel reusable review workflow plus its whole test harness on main. Two smaller correctness issues in the publish/withdraw steps are also worth fixing.
Findings: 1 blocking | 2 non-blocking | 0 posted inline
Blockers
- Branch base divergence makes the PR unmergeable as described. The head branch is based on
d895e04, which is not an ancestor ofmain, so the diff against the base branch is+6885 / -0across 43 files (.github/workflows/seidroid-review.yml,.github/workflows/workflow-test-self.yml, and all oftest/seidroid-review/**) rather than the four-line version bump the description names.maincurrently carriesai-review.ymlas its reusable reviewer; merging this would add a second, parallel one plus a self-test workflow that runs on everymainpush touching those paths. The author explicitly leaves the merge decision open ("whether this should ultimately merge intomainas-is ... is a call for whoever owns that restructuring"), so this cannot be merged on the strength of the stated payload. Rebase ontomainand apply the bump where the workflow actually lives now, or land it on the branch the five callers pin and keep it offmain.
Non-blocking
- Scope note: the description asks reviewers to read commit
92f74c9instead of the rendered diff. I reviewed the full diff against the base branch anyway, since that is what merging would apply. Unlike Codex, I do not read this as a prompt-injection attempt — it is a signed, transparent disclosure of the divergence aimed at human reviewers, and it discloses the problem rather than hiding it. The scope mismatch is captured as the blocker above instead. - Verified positives worth recording: the bump is applied at every site (
driver-versiondefault line 113,MIN_DRIVER_VERSIONline 1360, doc examples lines 106 and 1376) with no stalev0.17.0left as a live value; the guard passes all event-derived strings (comment/review bodies, actor logins) throughenv:rather than inline${{ }}inrun:, so there is no script-injection surface; the test stubs and harness scripts all carry the executable bit; andrun-guard.sh:571's cross-file assertion thatai-assistant.yml'strigger-phrasedefault is@seidroidstill holds against the merged tree.
Comments that couldn't be anchored to the diff
.github/workflows/seidroid-review.yml:2503-- [suggestion] AnAPPROVEcan attach to code the review never read. WhenRecord the commit under reviewfails (it iscontinue-on-error: true),head_shais empty andpost_positionomitscommit_id(line 2547); the reviews API then defaults to the pull request's latest commit. If a push lands during the review — exactly the race the recorded-head design at line 1702 exists to prevent — the approval attaches to that newer head, and a merge gate keyed on this reviewer reads as satisfied for unreviewed code.
The fallback is reasonable for REQUEST_CHANGES (a block on a newer head is conservative), but not for the approving direction. Suggest gating the APPROVE branch on a non-empty head_sha and letting it fall through to the COMMENT rung below when the commit is unknown:
elif [ "$conclusion" = "success" ] && [ "$APPROVE_ON_SUCCESS" = "true" ] && [ -n "$head_sha" ]; thenThe existing warning at line 2495 already explains why to the log.
.github/workflows/seidroid-review.yml:2649-- [suggestion] This dismissal query selects everyCHANGES_REQUESTEDreview whose body starts withVERDICT_MARKER, regardless of who authored it, and the loop below dismisses each one. Any review by another actor whose body happens to open with<!-- seidroid-review -->— a copied verdict body, a script that reuses the marker — gets silently withdrawn by a clean run, and the dismissal message claims the latest review superseded it.
The file already applies the author filter everywhere else it matches on this marker: line 1108 selects .user.type == "Bot", and so does line 3040. Adding the same predicate here (and, for consistency, at line 1070) makes the withdrawal only ever clear a block this workflow left:
select(.user.type == "Bot" and .state == "CHANGES_REQUESTED" and ((.body // "") | startswith($ENV.VERDICT_MARKER))).github/workflows/seidroid-review.yml:1348-- [nit] The rationale chain here stops at v0.17.0 and is not extended for the floor this PR sets. That matters because the chain is what justifies the floor at all — each entry names a conclusion the new driver reaches differently — while v0.18.0's changes are transport reliability fixes (token minting on the health-checked transport, session-lookup retry) that do not change any conclusion. A caller still pinned to v0.17.0 now hard-fails at line 1415 and reads an error that tells them their driver "reaches a different conclusion for the same findings," which is not true for this bump.
Raising both halves together is right per the invariant stated at lines 1354-1359 — the point is just that the comment should say why v0.18.0 is the floor, so the next reader can tell a conclusion-changing bump from a reliability one.
Contributor
Author
|
Pushed an annotated tag, `pin-seidroid-min-driver-v0.18.0`, pointing at this branch's tip (`92f74c9`) -- caught by seidroid's own review on one of the five caller PRs: relying on the branch alone means routine cleanup could delete it and silently break seidroid-review across all five repos at once, since their `uses:` lines pin this SHA directly. The tag is the safety net while this PR is open, not a replacement for landing it. Please don't delete this branch until #106 is either merged as-is or the five callers have moved off this SHA onto whatever it's superseded by -- the tag alone doesn't make the branch's deletion harmless, since a tag can be removed too; it just makes an accidental deletion less likely. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
Raises
seidroid-review.yml's driver floor fromv0.17.0tov0.18.0(sei-protocol/sei-internal-skills), which ships two transport fixes traced to a recurring ~20-29% no-verdict crash rate across every caller of this workflow: mint the machine-credential token on the health-checked transport instead of a bare client with no dead-connection detection (#420), and retry a session lookup that never reached the server instead of failing the run outright (#427).Moves both halves together per the file's own note: the
driver-versiondefault andMIN_DRIVER_VERSIONare one value in two places, and only raising both keeps a caller that omits the input (all five current callers) actually running the fixed driver rather than being admitted by a floor the default no longer matches.Important: this branch is NOT based on current
mainIt's based on
d895e04ea5aacc796c6655d4c82a3ac7271fb04d— the exact commit every current caller'suses:line pins.seidroid-review.ymlhas since moved onmainin ways that commit doesn't reflect (the file isn't present at this path onmainat all right now), and reconciling that is a separate, larger change than today's fix.This PR's diff against
mainwill look large and possibly like it's reintroducing a removed file — that's the divergence, not new content. The actual payload is the four-line version bump (threev0.17.0→v0.18.0occurrences plus the matching doc example), all against the file as it exists at the pinned commit. Please review the commit itself (92f74c9a68e4967c59b289b8e2632da7a63a6910) rather than the rendered PR diff againstmainfor the real change.Whether this should ultimately merge into
mainas-is, or the five callers should instead be migrated to wherever this workflow's functionality now lives onmain, is a call for whoever owns that restructuring — flagging it here rather than deciding it myself. Either way, this commit's own SHA is what the five callers' PRs (opening next) will reference.🤖 Generated with Claude Code