feat(seidroid-review)!: remove the cross-repository target grammar

[bdchatham]

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

[cursor]

PR Summary

Medium Risk
Workflow behavior change for anyone using cross-repo review syntax; guard and token scoping changes affect when reviews run and where they post, though callers and inputs are unchanged.

Overview
Breaking: @seidroid review no longer accepts owner/repo#N — reviews always run on the pull request in the repository where the comment or pull_request event fired.

The guard drops review_repo / review_pr outputs and the target regex; valid commands are whole-line @?seidroid review with optional close only. Comments that still name another repo get a ::notice refusal (so they do not fall through to the assistant) and should_run=false, with detection fixed to use grep -m1 + emptiness instead of grep -q under pipefail (which could SIGPIPE and skip the notice on large bodies).

Downstream steps always use github.repository and the event PR number for admit, driver session, thread recall, head SHA, inline placement, check runs, review position, and verdict posting. App installation tokens stay minted with owner + repositories pinned to the calling repo. Concurrency groups on that repo and PR, not a remote target. Inline placement no longer skips when the workflow token cannot reach another repository.

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

[github-actions]

A clean, complete withdrawal of the cross-repository target grammar — all 31 expression sites, the guard's parse block, the outputs, the concurrency key and the now-tautological Place findings condition are removed consistently, with no dangling references and no workflow_call input changes. One non-blocking concern: the explicit refusal that replaced the grammar is emitted as an ::error:: annotation on a run that isn't linked to the PR, and it doesn't stop ai-assistant.yml from posting the conversational reply it was written to pre-empt.

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

Blockers

• None at the file/PR level.

Non-blocking

• Verified the removal is complete and self-consistent: no review_repo / review_repo_name / review_pr / REPO_OWNER / target_re references remain anywhere in .github/workflows/, no in-repo caller or doc (.github/seidroid/ai-review/README.md) documented the withdrawn target grammar, and the workflow_call input surface is unchanged so no caller breaks.
• The named_repo capture correctly reuses the grep -m1 … || true idiom rather than grep -q; with set -o pipefail the || true inside the command substitution absorbs the SIGPIPE-derived 141, so the fix described in the PR body holds.
• Keeping repositories: alongside owner: on both create-github-app-token mints is the right call — dropping it would have widened the token to every repository the installation reaches.
• 1 suggestion(s)/nit(s) flagged inline on specific lines.

[github-actions]

[suggestion] The refusal doesn't reach the person who asked, and it doesn't achieve the goal the comment above it states.

::error:: writes an annotation onto this workflow run's summary. An issue_comment-triggered run is not attached to the pull request, so it appears in no check list and on no timeline — the requester would have to go find the Actions run to see it.

Meanwhile the outcome this block is meant to prevent still happens. ai-assistant.yml reserves only the body that normalizes to exactly @seidroid review (.github/workflows/ai-assistant.yml:107-110), so @seidroid review owner/name#123 is not reserved, is_review_command is false, and the conversational assistant posts its generic reply exactly as before. The net effect of this change is the chatty answer the comment calls "a worse signal than silence", plus an invisible annotation.

If the refusal is meant to be seen, it has to be a PR comment (the guard has permissions: {}, so this would need a scoped grant or to be deferred to a step that already holds a token). If an annotation is genuinely all that's wanted, the comment above should say that it's a log-only record rather than a user-facing refusal, since the stated justification for existing at all is the signal to the requester.

Separately, the pattern only matches the exact whole-line owner/name#N shape, so @seidroid review owner/name#123 please or @seidroid review other-repo#5 fall through with no annotation at all — worth noting in the comment if that narrowness is deliberate.

[bdchatham]

Both correct, and the second one names the sharper version: the comment justified the refusal with a benefit it does not deliver. Fixed in the latest commit.

You are right on both mechanisms. An issue_comment run attaches to no pull request, so the annotation reaches whoever opens the run and nobody else. And ai-assistant.yml reserves the body that normalises to exactly @seidroid review, so the target form was never reserved and still reaches the conversational assistant.

I kept the refusal and rewrote the justification rather than escalating it, because both of those match ai-review: there the target form was never a review command either, so an unreserved body reaching the assistant is the incumbent's behaviour, not a regression this branch introduces. Parity is the standard for this work, so matching it is the answer rather than exceeding it.

What the line now claims is only what it does: it names the reason in the one place a person debugging "why did my request do nothing" will look. If we later want the refusal to reach the requester, the change is to ai-assistant.yml's reservation, not here — and that is a behaviour change past this ticket.

[github-actions]

A faithful, mechanical withdrawal of the cross-repository target grammar: every review_repo/review_pr/target_re/REPO_OWNER reference is gone, each removed expression fallback collapses to exactly the value it produced when no target was named (concurrency group, Place findings condition, Post the verdict REPO/PR ternaries all verified identity-preserving), and both App-token mints keep repositories: so token scope is not widened. The only net-new logic is the refusal notice, which is correctly written (captured through grep -m1 … || true rather than a pipefail-sensitive grep -q, and it no longer interpolates comment-derived text into a workflow command); my remaining notes are about whether that refusal ever reaches the person who wrote the request.

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

Blockers

• None at the file/PR level.

Non-blocking

• The refusal's reachability depends on caller wiring that lives outside this repo. The header states routing is the caller's ("it reads the body and passes the mode"), so a caller whose dispatch condition matches only the exact @seidroid review body never invokes this workflow for @seidroid review owner/name#12 — and the new named_repo branch is then unreachable dead code. Worth confirming against at least one real caller, or noting in the input docs that callers must dispatch on a looser body match for the refusal to fire.
• The PR description and the code disagree on the annotation level: the Verification block says "one ::error:: and one should_run key each time" and the "Not changed, worth a look" section reasons about ::error:: not failing a step, while the merged code emits ::notice:: (correctly, per its own inline rationale). Worth correcting the description so a later reader does not go looking for an error annotation that is not emitted.
• The refusal regex restates the command grammar's ^[[:space:]]*@?seidroid[[:space:]]+review([[:space:]]+close)? prefix verbatim three lines below the grammar itself. The comment notes they are deliberately matched on the same anchor, but nothing enforces that; a future change to the accepted command shape has to be made in both places or the refusal silently stops covering the fall-through set.
• 1 suggestion(s)/nit(s) flagged inline on specific lines.

[github-actions]

[suggestion] This refusal is written only to the guard job's log. should_run=false follows, the review job is skipped, and the workflow concludes success — so the requester sees a green check and has to open the run and read annotations to find out anything was refused.

The comment above justifies the refusal by saying silence is not available, because ai-assistant.yml reserves only the exact bare body and "a chatty answer to this reads as a review that ran somewhere." But this code does not stop the chatty answer: ai-assistant.yml's route step compares the normalized body against exactly @seidroid review, so @seidroid review owner/name#12 still fails that check and still reaches the conversational assistant. Net effect on the PR page is unchanged from before this branch — a generic reply — with the refusal recorded where nobody looks.

Since the stated goal is that the requester learns their request was declined, the refusal needs a channel they will actually see: a reaction or a short comment on the triggering comment (the workflow already has that pattern in the "Post the verdict" / trigger-reaction steps), or widening the ai-assistant.yml reservation so the refusal lands where the request was written. The PR description flags this as out of scope, which is a fair call — but as merged, the block does not achieve the outcome its own comment claims for it.

[bdchatham]

Both correct, and the second one names the sharper version: the comment justified the refusal with a benefit it does not deliver. Fixed in the latest commit.

You are right on both mechanisms. An issue_comment run attaches to no pull request, so the annotation reaches whoever opens the run and nobody else. And ai-assistant.yml reserves the body that normalises to exactly @seidroid review, so the target form was never reserved and still reaches the conversational assistant.

I kept the refusal and rewrote the justification rather than escalating it, because both of those match ai-review: there the target form was never a review command either, so an unreserved body reaching the assistant is the incumbent's behaviour, not a regression this branch introduces. Parity is the standard for this work, so matching it is the answer rather than exceeding it.

What the line now claims is only what it does: it names the reason in the one place a person debugging "why did my request do nothing" will look. If we later want the refusal to reach the requester, the change is to ai-assistant.yml's reservation, not here — and that is a behaviour change past this ticket.

[github-actions]

Clean, complete removal of the cross-repository target grammar from seidroid-review.yml: all 3 guard outputs, the parse block, the dead REPO_OWNER env, the now-tautological placement condition and every consumer expression are gone, workflow_call inputs are untouched so no caller breaks, and both App-token mints keep repositories: alongside owner: so the token scope is not widened. No blocking issues; two minor notes.

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

Blockers

• None at the file/PR level.

Non-blocking

• The refusal for a repository-targeted request is a ::notice:: on an issue_comment run, which is only visible to someone who opens the Actions run. Meanwhile ai-assistant.yml still answers that body conversationally (its reservation is the exact @seidroid review alone), so the person who typed the withdrawn form sees a normal assistant reply and no indication the target was refused. The PR body acknowledges this tradeoff; worth revisiting if users start reporting "my review request did nothing", e.g. by teaching the assistant prompt about the withdrawn form so the reply the user actually sees says so.
• 1 suggestion(s)/nit(s) flagged inline on specific lines.

[github-actions]

[nit] The command prefix ^[[:space:]]*@?seidroid[[:space:]]+review([[:space:]]+close)? is now written out three times in this step (the cmdline= grep at 347, this refusal grep, and the close-detection grep at 391). The removed target_re variable existed to avoid exactly this; hoisting the shared prefix into a local (e.g. cmd_re='^[[:space:]]*@?seidroid[[:space:]]+review') would keep the refusal from silently diverging from the grammar it is supposed to mirror when the trigger word or an option is next changed. The comment above already promises they are "matched on the same whole-line anchor" — a shared variable is what makes that structurally true rather than true by inspection.

The SIGPIPE fix itself is right: grep -m1 … || true inside a command substitution with a -z/-n test matches the cmdline= idiom above and is not exposed to pipefail propagating 141.