feat(seidroid-review): widen the trigger to every comment event, and let a caller name it

[bdchatham]

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: fix(seidroid-review): withdraw the reactions on a cancelled run, from a step that cannot post #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

[cursor]

PR Summary

Medium Risk
Changes who can trigger reviews, which GitHub events are accepted, and how reactions/API paths are chosen—security-sensitive workflow wiring—but behavior is heavily covered by new guard/reaction harnesses and fail-closed admission checks.

Overview
Manual @seidroid review now works from PR conversation comments, diff-thread comments, and review bodies, not only issue comments. The guard parses comment.* or review.* depending on the event, restricts diff-side events to created/submitted, and refuses mis-wired events (including pull_request_target) with an explicit failing step so runs do not silently succeed.

Restores trigger-phrase (default @seidroid, shape-validated for safe grep) and allowed-bots (exact login allowlist; empty string falls back to []). Humans still pass via team membership; listed bots skip the team check but stay subject to fork/skip-label/command rules.

Reaction ack/answer/withdraw steps now use a new comment_api output (issues/comments vs pulls/comments); review bodies get no reactions endpoint, so trigger_id labels the driver while comment_id stays empty where nothing is reactable. The review job keeps both issues:write and pull-requests:write for those paths.

Adds a Guard the request CI job with run-guard.sh, gha.py (short-circuiting expression eval), and bin-guard/gh; extends reactions.sh for collection routing and ai-assistant.yml in workflow test paths so overlap with the assistant is asserted on all three comment events.

^{Reviewed by Cursor Bugbot for commit a31efa6. Bugbot is set up for automated code reviews on this repo. Configure here.}

[bdchatham]

One risk this change adds, for a reviewer to weigh: fromJSON(inputs.allowed-bots) sits in the guard's job condition, which is evaluated on every event including pull_request. A caller typo in that JSON therefore fails the automatic path too, not only the comment path. Moving the read into Admit the request alone would contain it, at the cost of spinning a runner for an unlisted bot — which is the thing ai-review's own note says must not happen.

Also: PR #100 edits Answer the request's if:, three lines above the env: block this branch adds COMMENT_API to. Whichever merges second conflicts there.

[github-actions]

Widening the seidroid-review guard to pull_request_review_comment / pull_request_review, restoring allowed-bots and trigger-phrase, and refusing unhandled events all look correct: the per-event payload reads are consistently comment.* || review.*, the reactions endpoint is now routed per event, and the new run-guard.sh harness covers the admission paths thoroughly. No blockers; a few suggestions around a stale comment, the double duty comment_id now serves, and two gaps in how the new harness is wired into CI.

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

Blockers

• None at the file/PR level.

Non-blocking

• contains(fromJSON(inputs.allowed-bots), ...) sits in the guard's job if, and the input docs promise that malformed JSON "refuses it rather than denying quietly". That promise depends on the PR's own stated-but-unmeasured assumption that GitHub evaluates both operands of ||. If GitHub short-circuits || (it is documented as returning the first truthy operand), a request from a human MEMBER would satisfy the person branch first, the fromJSON would never be reached, and the run would proceed to be denied by Admit the request instead — behaviourally still fail-closed, but not what the description or harness group 2 (a list that is not JSON -> error) asserts. Worth one real run with a deliberately broken allowed-bots to settle it, since gha.py cannot.
• Every previously-ignored event now hard-fails the guard rather than skipping it. That is the intent of PLT-1161, but any existing caller that wires e.g. workflow_dispatch or merge_group at the same call site will flip from a silent skip to a red run on upgrade. Worth a line in the release notes for this file.
• 4 suggestion(s)/nit(s) flagged inline on specific lines.
• 2 non-blocking pre-existing issue(s) listed below under pre-existing issues.

Pre-existing issues

• [suggestion] The guard's issue_comment branch has no github.event.action filter, unlike the two new events which are pinned to created/submitted. A caller that wires on: issue_comment without types: [created] can start a review by editing an old comment to add the phrase, and a deleted event still carries the body, so a review starts against a comment that no longer exists (the acknowledgement then reacts to a missing object). Present on the base branch; the PR only makes the asymmetry visible by constraining the other two events.
• [suggestion] ai-assistant.yml's reply job reserves only the exact body @seidroid review (github.event.comment.body != format('{0} review', inputs.trigger-phrase)), so @seidroid review close and any body carrying the command amid prose are answered by both the assistant and the reviewer. This is pre-existing on issue_comment; note that it now applies on the diff-thread and review-body paths as well.

[github-actions]

[suggestion] comment_id now serves two consumers with different needs, and this change only reasons about one of them.

It is the reactable-object id for Acknowledge the trigger / Answer the request — correct to blank it when there is no reactions endpoint. But it is also --trigger-id on the driver (if [ -n "${TRIGGER_ID:-}" ]; then args+=(--trigger-id "$TRIGGER_ID"); fi in Drive session + collect verdict). Two consequences:

• A review-body trigger silently stops passing --trigger-id to the driver, even though review.id is known and was read into COMMENT_ID two steps up.
• A diff-thread trigger now passes a pull request review comment id where the driver previously always received an issue comment id. Those are separate id spaces, so a value that used to identify a conversation comment can now collide with an unrelated one.

If --trigger-id is only a log discriminator both are harmless, but that is not stated anywhere here. Consider emitting the reactable id as its own output (e.g. reaction_id, paired with comment_api) and keeping comment_id as the always-populated trigger id.

[bdchatham]

Taken, in the smaller form you suggested. The guard emits trigger_id — always populated, on every comment event — beside comment_id, and only Drive session + collect verdict moved to it. No step condition changed, so conditions.py's context model did not have to learn a new key.

On the two consequences: the first was real and is fixed — a review-body dispatch had silently stopped passing --trigger-id. The second is bounded by the fact that --trigger-id only labels the dispatch in the log, which the file does state, three lines below the code you flagged. A new group asserts which of the four outputs each consumer reads, so the driver taking the reactable id again fails an assertion.

[github-actions]

[suggestion] Group 16 of run-guard.sh reads .github/workflows/ai-assistant.yml and asserts a cross-workflow invariant (its trigger-phrase default equals this one's, and which of the two answers a given body). But this workflow's paths: filters only list seidroid-review.yml, workflow-test-self.yml and test/seidroid-review/** — so a PR that changes ai-assistant.yml's trigger phrase or its reply condition, which is exactly the drift group 16 exists to catch, never runs this job. Add .github/workflows/ai-assistant.yml to both paths: lists.

[github-actions]

[suggestion] claims() hardcodes event_name: "issue_comment" and only populates event.comment, so group 16 measures the ai-assistant overlap on the one event that already had it. ai-assistant.yml's reply condition has parallel branches for pull_request_review_comment and pull_request_review with the same contains(body, phrase) && body != '<phrase> review' reservation — so this PR newly widens the double-answer cases (@seidroid review close, a command amid prose) onto the two events it adds, and none of that is covered. Parameterising claims() by event and adding the two review-side contexts would make the widened overlap measured rather than inferred, which is the stated point of the group.

[bdchatham]

Taken. claims() is parameterised by event, every body runs on all three, and an empty review body is added — the case ai-assistant.yml names and this workflow reads as no command.

The overlap turns out identical on all three events, which is now measured rather than inferred. Group 16 went from 11 assertions to 33.

[github-actions]

[nit] "No comment event carries a head repository" is not accurate for two of the three. pull_request_review_comment and pull_request_review payloads both carry a full pull_request object, including pull_request.head.repo.id and pull_request.base.repo.id — only issue_comment does not. The code is still correct because the payload branch is gated on EVENT_NAME = pull_request, but as written the comment justifies an API call that two of the three comment paths do not actually need, and the HEAD_REPO_ID / BASE_REPO_ID env comment above ("Empty on every other event") is wrong for the same two events.

[github-actions]

Widening the seidroid-review guard to pull_request_review_comment / pull_request_review, restoring allowed-bots and trigger-phrase, and refusing unhandled events all look correct: every payload read is dual-keyed (comment.* or review.*), the reaction collection is routed per event with comment_id held back where no endpoint exists, and the new bin-guard/gha.py harness covers the paths well. No blocking issues; two notes about test fidelity and CI trigger coverage, plus one stale comment.

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

Blockers

• None at the file/PR level.

Non-blocking

• .github/workflows/seidroid-review.yml:1204-1206 (unchanged line, but now incomplete because of this PR): the permissions: comment still says a reaction on a PR comment "goes to the ISSUE comments endpoint, which pull-requests: write does not cover". With comment_api now able to be pulls/comments, the diff-thread acknowledgement reaches POST /repos/{o}/{r}/pulls/comments/{id}/reactions, which needs pull-requests: write and not issues: write. Both are granted so nothing breaks today, but anyone trimming permissions on the strength of that comment would silently break diff-thread acks. Worth extending it to name both collections.
• test/seidroid-review/gha.py hardcodes cancelled() → false and success() → true, so the !cancelled() clause the review job's condition rests on is never exercised in either direction by group 4. Not worth modelling job status generally, but a one-line note in the docstring about what the model cannot answer would keep a later reader from over-trusting group 4.
• 2 suggestion(s)/nit(s) flagged inline on specific lines.

[github-actions]

[suggestion] or_ evaluates the right operand unconditionally (right = self.and_() runs before the truthiness test), and and_ does the same on line 103. GitHub's expression evaluator short-circuits: its Or/And nodes return on the first truthy/falsy operand and never evaluate the rest, so an error in an unreached operand never surfaces.

That divergence is load-bearing for one shipped assertion. run-guard.sh:190 feeds issue_comment / alice / User / MEMBER with allowed-bots: 'not json' and asserts the guard condition yields error. On a real runner the person branch — (...user.type) != 'Bot' && contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), ...author_association) — is already true, so fromJSON(inputs.allowed-bots) is never evaluated and the condition is simply true: the guard runs and admits the member. The malformed value would only fail the expression when the requester is a bot or a non-member, i.e. exactly the requests that were going to be denied anyway.

The production behaviour is still safe (Admit the request denies on unparseable allowed-bots, and run-guard.sh:379 covers that), so this is a test-fidelity issue rather than a guard defect. Suggest either modelling short-circuit evaluation and re-deriving the expected value for that case, or dropping the error expectation and stating in the docstring that eager evaluation is a deliberate over-approximation.

[bdchatham]

Correct, and it caught a claim in my own report: I had listed "GitHub evaluates both operands of ||" as read-not-measured, and had it backwards.

gha.py short-circuits now — Or and And walk the unreached operand for its tokens but hold back its function calls, which is where fromJSON would refuse. Exactly the one assertion you predicted flipped, and only that one. It is re-derived per requester: a person yields true (admitted on association before the list is read), a bot yields error (the requester whose admission depends on parsing it), and an automatic review yields true. The docstring and --selftest both state it, 18 readings now.

[github-actions]

[suggestion] run-guard.sh reads a second workflow that this trigger does not watch: group 16 asserts ai-assistant.yml's trigger-phrase default equals @seidroid (line 523) and evaluates that file's reply job condition (line 529). The paths: filters above only list seidroid-review.yml, workflow-test-self.yml and test/seidroid-review/**, so a PR that changes ai-assistant.yml's default phrase or its reply condition will not run this job — the non-overlap claim the PR describes as "measured rather than argued" would go back to being unmeasured, and the break would only show up on some later unrelated PR. Add .github/workflows/ai-assistant.yml to both paths: lists.

[github-actions]

Widening the seidroid-review guard to pull_request_review_comment / pull_request_review, restoring allowed-bots and trigger-phrase, and refusing unhandled events all look correct: every payload read is dual-keyed, the reaction collection is resolved per event and threaded through all three reacting steps (which already gate on comment_id), and the new run-guard.sh/gha.py harness covers the admission path well. No blocking issues; three non-blocking notes on caller-input robustness, a test-trigger gap, and two now-stale comments.

Findings: 0 blocking | 5 non-blocking | 3 posted inline

Blockers

• None at the file/PR level.

Non-blocking

• review job permissions comment (seidroid-review.yml:1207-1213) is now half the story: it explains only why issues: write is needed for issues/comments/{id}/reactions. The diff-thread path reacts on pulls/comments/{id}/reactions, which is covered by pull-requests: write instead. Worth a sentence so nobody prunes either scope on the strength of that comment alone.
• README.md ends without a trailing newline.
• 3 suggestion(s)/nit(s) flagged inline on specific lines.

[github-actions]

[suggestion] fromJSON(inputs.allowed-bots) sits in the job condition, so a value it cannot parse fails the expression and takes the whole run down rather than denying. The documented intent is "malformed JSON is a caller error", but the most likely accident is an empty value, not malformed JSON: workflow_call defaults apply only when the input is omitted, so a caller wiring allowed-bots: ${{ vars.SEIDROID_ALLOWED_BOTS }} with the variable unset passes '', and fromJSON('') is not [].

That asymmetry is worth closing — Admit the request reads the same value through jq and simply denies, and trigger-phrase/guidelines-file both warn-and-fall-back. A one-token fix keeps the loud failure for genuinely malformed JSON while treating an unset input as the documented default:

contains(fromJSON(inputs.allowed-bots || '[]'),

[bdchatham]

Taken. The condition reads fromJSON(inputs.allowed-bots || '[]') now, so an unset input 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: the sibling finding on gha.py showed my eager model was wrong, and under the runner's real short-circuiting a bad list only ever reached a bot request. Four cases pin it — empty denies a bot even with MEMBER association, admits a person, and leaves an automatic review alone.

[github-actions]

[suggestion] run-guard.sh group 16 asserts a cross-file invariant against .github/workflows/ai-assistant.yml — that its trigger-phrase default is still @seidroid, and what its reply condition claims of each body. But ai-assistant.yml is not in either paths: filter above, so editing that file runs none of this. The invariant can break silently and then surface as a red Guard the request check on the next unrelated seidroid-review.yml change, pointing at the wrong PR.

Adding '.github/workflows/ai-assistant.yml' to both paths: lists makes the check fire where the breakage happens.

[bdchatham]

Taken — ai-assistant.yml is in both paths: lists.

I audited every file the three harnesses read, as you asked. It was the only one outside the filter: run.sh and reactions.sh read seidroid-review.yml alone, and conditions.py takes its target from the CI command line, which names the same watched file. So no other cross-file assertion has this shape.

[github-actions]

[nit] "No comment event carries a head repository" holds for issue_comment only. The pull_request_review_comment and pull_request_review payloads both carry the full pull_request object, including head.repo and base.repo — which is why HEAD_REPO_ID/BASE_REPO_ID (lines 833-834) are in fact populated on those two events and then deliberately ignored by the EVENT_NAME = pull_request branch below.

No behavioural bug: the API read returns the same answer. But the two new paths spend an avoidable authenticated call per request, and the comment states something about the payload shape that a later reader would be wrong to trust. Either widen the payload branch to "a payload that carries a base repository id" (which would cover all three) or correct the sentence to name issue_comment as the one event that needs the API.

[bdchatham]

Corrected the sentence rather than widening the branch, and the reason decided it: 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, so the saving is a round trip, not a refused review.

The comment now names issue_comment as the one event that needs the API, says the other two carry head.repo.id and base.repo.id, and records what a payload-keyed branch would have to preserve — so the saving stays available to whoever wants it.

The bigger one is collapsing those two reads of the same endpoint into one, which is available on all four paths rather than two. That belongs in a ticket: it moves the fork check, which is the highest-consequence control in the guard, and I would rather not move it in a change that has rebased three times.

[bdchatham]

Taken. The permissions comment explains both scopes now, one per collection: issues/comments/{id}/reactions under issues: write and pulls/comments/{id}/reactions under pull-requests: write, with the consequence of pruning either named — the reaction fails on the path that scope serves, and all three reacting steps treat a lost reaction as a courtesy and only warn.

Separately, on the measurement discipline: five processes were reported running concurrently. They were my five polling shells, not sweeps — pgrep -f mutate.py matches any command line that mentions the script, including the loop asking the question, so my waits could never terminate and the count read five when the true count was zero. Two replacement predicates were also wrong: [m]utate\.py still matched a poller whose echo named the script, and ps -eo comm,command truncates comm to /opt/homebrew/Ce, so it matched nothing. scratchpad/triggers/sweeps.sh matches ARGV0 instead and was validated against a process I could see.

The instruction stands regardless and I followed it: every poller killed, count verified zero, tree confirmed identical to a31efa6, and one sweep run serially with the count checked before launch. mutate.py also refuses to start on a dirty workflow now — measured, not asserted — so a second concurrent sweep exits rather than reading another's mutation as its baseline.

[github-actions]

Widens the seidroid-review guard to pull_request_review_comment and pull_request_review, restores allowed-bots and trigger-phrase, and refuses unsupported events — all backed by a new 241-assertion harness that evaluates the shipped conditions and step scripts rather than restating them. The per-event payload reads, the comment_api split (issues/comments vs pulls/comments), the comment_id/trigger_id separation, and the fail-closed bot check all check out; only two non-blocking points.

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

Blockers

• None at the file/PR level.

Non-blocking

• gha.py models cancelled()/failure() as constant false and success()/always() as constant true. That's fine for the guard/review admission conditions it is used on, but the module docstring lists them alongside the genuinely modelled operators without saying they are fixed, so a later assertion written against !cancelled() would pass vacuously. A one-line note next to apply_function would keep the next author from trusting it further than it goes.
• 2 suggestion(s)/nit(s) flagged inline on specific lines.
• 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.

Pre-existing issues

• [suggestion] .github/workflows/seidroid-review.yml guard condition: the issue_comment branch is not held to github.event.action == 'created', unlike the two branches this PR adds. issue_comment also fires on edited and deleted, and both payloads carry the full comment body — so editing (or deleting) a comment that already contains @seidroid review re-runs the review. This is exactly the replay hazard the PR guards against for pull_request_review.dismissed; the workflow's comment notes the asymmetry ("issue_comment is left as it stands") but does not close it. Callers that wire types: [created] themselves are unaffected.

[github-actions]

[suggestion] trigger-phrase is now caller-settable and the grep pattern is built from it, but two runtime notices still hardcode the default:

• line 1103: deny "... comment @seidroid review to ask for one"
• line 928: ... An automatic pull_request review and @seidroid review close do not reach this check"

A caller that sets trigger-phrase: '@mybot' gets told to type a phrase that this workflow's own parse will not match — and line 1103 is the primary "how do I ask for a review" guidance, emitted on the once-per-PR deny path. Both steps already have the phrase available (parse reads it into TRIGGER_PHRASE; Admit the request would need it added to env:), so interpolating it there would close the gap. PLT-1164 built the pattern from the input; these two messages are the remaining hardcoded readings of it.

[github-actions]

[suggestion] Worth reconsidering now that the trigger reaches diff threads and review bodies. The comment above says whole-line anchoring keeps a comment that discusses the command from matching because it "has other words on the line" — but that does not hold for a markdown code block. A fenced line @seidroid review, or a 4-space-indented @seidroid review, matches ^[[:space:]]*@?${phrase}... exactly, and GitHub renders it without notifying anyone, so the author gets no hint they fired a review.

This existed on issue_comment, but the two new events move it to where it is most likely to fire: a diff-thread comment or review body on this very workflow file or on test/seidroid-review/README.md, both of which quote the command verbatim. The cost is a real sandbox and model spend per occurrence; the concurrency group only collapses concurrent ones.

If you want to keep the optional @ (and the PR's argument for it is reasonable), rejecting a line indented by four or more spaces would remove the indented-block case cheaply. Fenced blocks would need the parse to track ``` state, which is a bigger change and may not be worth it.