-
Notifications
You must be signed in to change notification settings - Fork 2
ci: wire the seidroid review workflow #251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
acbbb4d
ci: wire the seidroid review workflow
b88a460
ci: state the reusable-workflow scope rule precisely
0a184c1
ci: correct seven inaccurate comments in the seidroid caller
0dc9768
ci: stop claiming close is the only path that deletes a session
907f938
ci: correct the cancellation semantics, from the driver not the callee
ed28fd4
ci: name all three axes of the caller/callee grammar mismatch
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,175 @@ | ||
| # Wiring only. The review logic lives in sei-protocol/uci and the reviewer binary in | ||
| # sei-protocol/sei-internal-skills; both document their own behaviour and inputs. | ||
| # | ||
| # @seidroid review review this pull request | ||
| # @seidroid review close destroy this pull request's session now | ||
| # | ||
| # A review request is read from a top-level comment, an inline review comment or a | ||
| # review body. A teardown is read from a top-level comment only. | ||
| # | ||
| # Four things here diverge from this repository's other workflows on purpose. Each is | ||
| # commented where it appears: the sha pin below, the lowercase `name:`, the empty | ||
| # top-level `permissions:`, and the absence of a `concurrency:` block. | ||
| # | ||
| # The `uses:` lines pin a raw commit sha, where uci-lint.yml, uci-release-check.yml, | ||
| # uci-release-publish.yml and uci-stale-check.yml each pin a uci tag. That is | ||
| # availability, not preference: this callee is on no uci tag and on no uci default | ||
| # branch -- the workflow file has no history on uci's `main` -- so there is no tag to | ||
| # name. Every other repository wiring this reviewer pins the same sha, and this file is | ||
| # bumped with them by path, so keep the three occurrences below identical. | ||
| # | ||
| # Lowercase, where this repository Title-Cases (`Go Linter`, `Releaser`, `Stale | ||
| # Checker`). `seidroid review` is the literal command a person types, and the callee | ||
| # sets `run-name: UCI / seidroid review / #<n>`; Title-Casing it would put this | ||
| # workflow's name at odds with its own trigger phrase and with every other caller. | ||
| name: seidroid review | ||
|
|
||
| on: | ||
| issue_comment: | ||
| types: [created] | ||
| # The two review events as well, so `@seidroid review` reaches the pipeline when it | ||
| # is typed where the discussion already is -- inside an inline diff thread or in a | ||
| # review body, rather than only on the conversation. Convenience, not capability: a | ||
| # top-level `issue_comment` already asks for a re-review of the current tree. The | ||
| # callee admits both events by name and applies the same author checks to them. | ||
| # Wired here after platform, which added them first. | ||
| pull_request_review_comment: | ||
| types: [created] | ||
| pull_request_review: | ||
| types: [submitted] | ||
| pull_request: | ||
| types: [opened, ready_for_review, synchronize, reopened, closed] | ||
|
|
||
| # Empty, where this repository's other workflows grant scopes at the top level. A | ||
| # reusable workflow may only downgrade what its caller granted, so the caller sets the | ||
| # ceiling. Sufficient inherited permissions would serve too; the choice here is to grant | ||
| # the required scopes explicitly rather than depend on a configured default. Empty at the | ||
| # top means no job inherits one, and each job then states exactly the scopes the callee's | ||
| # two jobs declare between them, so what this caller grants does not move with whichever | ||
| # GITHUB_TOKEN default the organisation or the repository has set. | ||
| permissions: {} | ||
|
|
||
| # No `concurrency:` block, where all four uci callers in this repository set one with | ||
| # `cancel-in-progress: true`. Three reasons, in order: | ||
| # | ||
| # - The callee already groups the way we would. Its review job carries its own | ||
| # job-level `concurrency`, keyed on mode + repository + pull request number with | ||
| # `cancel-in-progress: true`, so a newer `@seidroid review` cancels an in-flight | ||
| # review while a teardown for the same pull request sits in a separate group and | ||
| # survives. A caller group on top of that could only narrow it. | ||
| # - The grouping a caller would reach for first is the wrong one. A workflow-and-pull | ||
| # request group with no mode in the key would put the review job and the reclaim job | ||
| # together, so a review arriving on a just-closed pull request could cancel an | ||
| # in-flight reclaim -- and `mode: close` is the path that reclaims a session's | ||
| # sandbox. Mode has to stay in the key, and the callee already keeps it there. | ||
| # - Cancelling a review frees nothing, and stops less than the word suggests. | ||
| # Cancellation unwinds the driver without deleting the session: the conversation | ||
| # stays, the sandbox stays allocated until something reclaims it, and the remote | ||
| # turn may keep running -- the next request queues behind that turn rather than | ||
| # racing it. So a caller-level `cancel-in-progress` would buy latency, not | ||
| # reclamation. `mode: close` is the explicit end-of-work path that reclaims the | ||
| # sandbox. Close is not the only code path that can delete a session -- opening a | ||
| # review deletes and replaces one it finds unable to run a turn at all -- but that | ||
| # repairs an already-dead session rather than ending a live unit of work, so it is | ||
| # not a teardown a caller can schedule. | ||
| # | ||
| # What the callee's grouping is NOT: exactly-once, or a session lifecycle. A push and a | ||
| # comment can each admit a review of the same tree; overlapping runs cancel, sequential | ||
| # requests are not deduplicated. Review and close sit in separate groups by design, and | ||
| # the comment path does not test whether the pull request is already closed. | ||
| jobs: | ||
| seidroid-review: | ||
| # Substring tests, with the teardown phrase excluded here so `@seidroid review | ||
| # close` asks for exactly one thing. The callee does not match substrings: it | ||
| # requires a whole LINE reading `@seidroid review [close]`, and it never checks the | ||
| # line it parsed against `mode`. A body mixing both commands can therefore route to | ||
| # the wrong one -- the PR description carries the case that matters. Not fixable | ||
| # here: GitHub expressions have no regex, so a caller-side grammar would trade one | ||
| # set of wrong answers for another, and the only reader that can compare a parsed | ||
| # line to `mode` is the callee. | ||
| if: >- | ||
| (github.event_name == 'pull_request' && | ||
| github.event.action != 'closed') || | ||
| (((github.event_name == 'issue_comment' && | ||
| github.event.issue.pull_request != null) || | ||
| github.event_name == 'pull_request_review_comment' || | ||
| github.event_name == 'pull_request_review') && | ||
| contains(github.event.comment.body || github.event.review.body, | ||
| '@seidroid review') && | ||
| !contains(github.event.comment.body || github.event.review.body, | ||
| '@seidroid review close')) | ||
| uses: sei-protocol/uci/.github/workflows/seidroid-review.yml@d895e04ea5aacc796c6655d4c82a3ac7271fb04d | ||
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
| checks: write | ||
| issues: write | ||
| secrets: | ||
| OMNIGENT_MACHINE_CLIENT_SECRET: ${{ secrets.OMNIGENT_MACHINE_CLIENT_SECRET }} | ||
| SEIDROID_APP_ID: ${{ secrets.PLATFORM_CODE_AGENT_APP_ID }} | ||
| SEIDROID_APP_PRIVATE_KEY: ${{ secrets.PLATFORM_CODE_AGENT_APP_PK }} | ||
| with: | ||
| mode: review | ||
| # A choice for this repository, not something inherited with the file: on a clean | ||
| # conclusion the callee ATTEMPTS an approving review. Where the API reports a 422 | ||
| # it attempts a COMMENT review instead; a refusal that arrives any other way -- a | ||
| # 403 from an approval policy, say -- gets no second attempt, and a post that does | ||
| # not succeed leaves a warning. So this input buys an attempt, not a guaranteed | ||
| # approval. Whether an approval that does land counts toward a required one is a | ||
| # matter of branch policy and reviewer eligibility, neither of which this file | ||
| # decides. Revisit this line when that policy changes. | ||
| approve-on-success: true | ||
| allowed-team: 'sei-protocol/sei-core' | ||
| # Unset reads REVIEW.md, which this repository does not keep. | ||
| guidelines-file: CLAUDE.md | ||
|
|
||
| # Anchored, unlike the review job's `contains`, and what that buys is narrow: a body | ||
| # whose first characters are not the teardown phrase does not select this job, so the | ||
| # phrase occurring LATER in a body does not start a teardown from here. It buys | ||
| # nothing else -- a body that begins with the phrase routes here whatever follows it, | ||
| # which is the case the PR description records. The review job stays on `contains` for | ||
| # fleet parity; re-anchoring it would not have prevented that case in any event, since | ||
| # it already excludes itself there. | ||
| seidroid-review-close: | ||
| if: >- | ||
| github.event_name == 'issue_comment' && | ||
| github.event.issue.pull_request != null && | ||
| startsWith(github.event.comment.body, '@seidroid review close') | ||
| uses: sei-protocol/uci/.github/workflows/seidroid-review.yml@d895e04ea5aacc796c6655d4c82a3ac7271fb04d | ||
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
| checks: write | ||
| issues: write | ||
| secrets: | ||
| OMNIGENT_MACHINE_CLIENT_SECRET: ${{ secrets.OMNIGENT_MACHINE_CLIENT_SECRET }} | ||
| SEIDROID_APP_ID: ${{ secrets.PLATFORM_CODE_AGENT_APP_ID }} | ||
| SEIDROID_APP_PRIVATE_KEY: ${{ secrets.PLATFORM_CODE_AGENT_APP_PK }} | ||
| with: | ||
| # Deletes the review session and reclaims its sandbox. It does not close the | ||
| # pull request. | ||
| mode: close | ||
| allowed-team: 'sei-protocol/sei-core' | ||
|
|
||
| # Does not cover a fork: GitHub withholds secrets from a `pull_request` run whose head | ||
| # is a fork, so this close cannot mint a credential. What that leaves is narrower than | ||
| # it looks -- the callee refuses a review on a fork-originated pull request from every | ||
| # trigger, so no routine path creates a session for one, and this covers a pre-existing | ||
| # session created outside these guarded review paths rather than a leak this wiring | ||
| # causes. Where one does exist, `@seidroid review close` reclaims it; the | ||
| # guard takes a teardown only from an author it admits as OWNER, MEMBER or COLLABORATOR, | ||
| # so ask an admitted collaborator or a maintainer if your own association is not one. | ||
| seidroid-review-reclaim: | ||
| if: github.event_name == 'pull_request' && github.event.action == 'closed' | ||
| uses: sei-protocol/uci/.github/workflows/seidroid-review.yml@d895e04ea5aacc796c6655d4c82a3ac7271fb04d | ||
| permissions: | ||
| contents: read | ||
| pull-requests: write | ||
| checks: write | ||
| issues: write | ||
| secrets: | ||
| OMNIGENT_MACHINE_CLIENT_SECRET: ${{ secrets.OMNIGENT_MACHINE_CLIENT_SECRET }} | ||
| SEIDROID_APP_ID: ${{ secrets.PLATFORM_CODE_AGENT_APP_ID }} | ||
| SEIDROID_APP_PRIVATE_KEY: ${{ secrets.PLATFORM_CODE_AGENT_APP_PK }} | ||
| with: | ||
| mode: close | ||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion — Unchanged since the last review, and still worth the comment fix. Verified again against the callee at the pinned sha: the guard's parse step greps for a whole line matching
^\s*@?seidroid\s+review(\s+close)?\s*$and writesshould_run=truewithout ever comparing the line it matched toinputs.mode, so this expression alone decides review-vs-close. The destructive case the description records reproduces exactly as written, and the review job's!containson line 99 is what makes the misroute exclusive rather than additive. That much the comment above already says.It is short on two further axes, and both widen the trigger surface beyond what a reader of the comment or the description would expect.
CASE. GitHub's
startsWithandcontainsare documented as case-insensitive; the callee'sgrep -Eis case-sensitive, and thetrigger-phraseinput says so explicitly (@Seidroid reviewstarts no review). A body opening@Seidroid review close ...therefore routes to this job, and if it also carries a properly-cased@seidroid reviewline the callee proceeds undermode: close. The mirror case is benign: a lone@SEIDROID REVIEWselects the review job and parses to nothing.WORD BOUNDARY.
startsWithtests a string prefix, not a command token, so@seidroid review closely at the retry logicand@seidroid review close-not-nowboth select this job. Alone each is a silent no-op — the whole-line grep rejects the line — but with a later bare@seidroid reviewline each is the destructive path, and the author has typed nothing resembling the teardown command anywhere in the body. The description's guidance ("don't begin a comment with the teardown phrase unless you mean it") does not prepare a reader for that: the phrase was never typed.Agreed the fix belongs in the callee and that inventing caller-side grammar here would fight the next fleet-wide pin bump; no change to the expression is being asked for. What is worth changing is the comment at line 82, which presents the divergence as substring-vs-whole-line only. Naming all three axes would let the next person weighing a caller-side fix see the full shape of what they would be fixing.