Skip to content

Enable seidroid xreview, bump uci to v0.0.15 - #85

Open
amir-deris wants to merge 1 commit into
mainfrom
amir/plt-873-add-seidroid-xreview
Open

Enable seidroid xreview, bump uci to v0.0.15#85
amir-deris wants to merge 1 commit into
mainfrom
amir/plt-873-add-seidroid-xreview

Conversation

@amir-deris

@amir-deris amir-deris commented Jul 31, 2026

Copy link
Copy Markdown

Enables seidroid xreview on sei-go-ethereum — the on-demand, sandbox-backed agentic review, a third seidroid capability alongside the ai-review / ai-assist this repo already runs. Also bumps those two existing workflows to the same uci release for consistency.

Linear: PLT-873

What this adds

New file .github/workflows/seidroid-xreview.yml — a thin caller that uses: the uci reusable workflow, so the review logic lives in sei-protocol/uci:

  • Pinned to the uci v0.0.15 commit SHA (6590124) on both uses: and uci-ref — a fixed ref, never a moving tag.
  • Passes only OMNIGENT_M2M_CLIENT_SECRET (not secrets: inherit), so the cross-repo reusable workflow never receives this repo's other secrets.
  • A local if: gate mirrors the reusable workflow's trust check (PR comment, non-bot, OWNER/MEMBER/COLLABORATOR, command mentioned) so an unrelated or untrusted comment never dispatches it.
  • Comment seidroid xreview on a PR to trigger it; the verdict posts as a single sticky comment.

What this changes

ai-review.yml and ai-assist.yml bumped from uci v0.0.13 (29a9c73) to v0.0.15 (6590124), same commit as xreview, so all three workflows track one release.

Prerequisites (need verification before this is live)

  • sei-droid GitHub App installed on this repo
  • uci-default runner-group access granted to this repo
  • OMNIGENT_M2M_CLIENT_SECRET secret configured (not currently visible at the repo-secret level — only CODECOV_TOKEN shows; may be an org-level secret, or may still need adding)

Test plan

  • Confirm sei-droid app + runner-group access + OMNIGENT_M2M_CLIENT_SECRET are in place
  • Open a test PR, comment seidroid xreview, confirm sticky verdict comment is posted
  • Confirm ai-review / ai-assist still run correctly on the bumped uci ref

Enables seidroid xreview (on-demand, sandbox-backed agentic PR review
via `seidroid xreview` comment), and brings ai-review/ai-assist onto
the same uci release for consistency.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@amir-deris amir-deris self-assigned this Jul 31, 2026
@amir-deris amir-deris changed the title TBC Enable seidroid xreview, bump uci to v0.0.15 Jul 31, 2026
@amir-deris
amir-deris marked this pull request as ready for review July 31, 2026 08:30
@cursor

cursor Bot commented Jul 31, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Introduces a cross-repo reusable workflow with M2M auth and PR write access; mitigations (SHA pin, scoped secret, author gate) reduce but do not eliminate CI/supply-chain and secret-exposure risk.

Overview
Adds on-demand seidroid xreview via a new .github/workflows/seidroid-xreview.yml caller: PR comments containing seidroid xreview from trusted non-bot authors dispatch the uci reusable workflow (pinned to commit 6590124 / v0.0.15), with a local if: gate mirroring the reusable workflow’s trust checks. The job passes only OMNIGENT_M2M_CLIENT_SECRET (not secrets: inherit), limits permissions (no id-token), and posts the verdict as a sticky PR comment.

ai-review and ai-assist are updated from uci v0.0.13 to v0.0.15 on the same commit SHA so all three seidroid workflows share one uci release.

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

@amir-deris
amir-deris requested review from bdchatham and masih July 31, 2026 08:30

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI-only change adding a thin caller workflow for seidroid xreview plus a uci v0.0.13→v0.0.15 SHA bump on the two existing AI workflows; the security posture (full-SHA pins, scoped secrets: rather than inherit, minimal permissions, local trust gate) is sound and I found no blockers. Remaining notes are non-blocking: a substring command match, no caller-level concurrency, an unverified/absent trigger secret, and secrets: inherit left in place on the two bumped workflows.

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

Blockers

  • None at the file/PR level.

Non-blocking

  • The Cursor second-opinion pass produced no output (cursor-review.md is empty), so that perspective is missing from this synthesis. Codex reported no material issues. REVIEW_GUIDELINES.md is also empty, so no repo-specific standards were applied.
  • Consistency: the new file's comment argues (correctly) that secrets: inherit would hand a cross-repo reusable workflow every secret in this repo. That same reasoning applies to ai-review.yml:14 and ai-assist.yml:18, which still use secrets: inherit and are unchanged here. Worth a follow-up to narrow those to the specific secrets uci actually needs — not required for this PR.
  • I could not verify from this environment that 65901242783550521f25a19199a6b10e54550b97 is in fact the tip of uci v0.0.15, nor that the callee declares only uci-ref as an input and only OMNIGENT_M2M_CLIENT_SECRET as a secret. If the callee declares any other required: true secret or input, every run fails at dispatch. Please confirm against the uci repo before merge.
  • The job comment (lines 12–17) asserts behavior of the external workflow — that it does an authoritative exact-command check and owns a job-level concurrency group. Those claims can silently drift as uci is bumped. Consider trimming to what this file controls, or noting the uci version the claims were verified against.
  • 4 suggestion(s)/nit(s) flagged inline on specific lines.

${{ github.event.issue.pull_request
&& github.event.comment.user.type != 'Bot'
&& contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association)
&& contains(github.event.comment.body, 'seidroid xreview') }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] contains(...) is an unanchored substring match, so any comment that merely mentions the command dispatches a run — e.g. a reviewer writing "you can run seidroid xreview on this", or a quoted reply (> seidroid xreview) — as long as the commenter is OWNER/MEMBER/COLLABORATOR. Not a security hole (the callee does the authoritative exact-command check, per the comment above), but it burns a runner slot and shows a spurious check on the PR.

Suggest anchoring it, which also makes the local pre-filter match the callee's intent more closely:

&& startsWith(github.event.comment.body, 'seidroid xreview')

types: [created]

jobs:
xreview:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] No concurrency group is declared at the caller level. The comment above states overlapping runs are handled by the reusable workflow's job-level group, but that is an external guarantee this repo can't enforce and that can drift on the next uci bump. A cheap belt-and-braces addition:

  xreview:
    concurrency:
      group: seidroid-xreview-${{ github.event.issue.number }}
      cancel-in-progress: true

This also caps damage from the substring match above if someone posts the command several times in a row.

# Pass ONLY the one secret xreview needs — not `secrets: inherit`, which would hand the
# cross-repo reusable workflow every secret on this repo (release, ECR/GHCR, Slack, …).
secrets:
OMNIGENT_M2M_CLIENT_SECRET: ${{ secrets.OMNIGENT_M2M_CLIENT_SECRET }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[suggestion] Per the PR description, OMNIGENT_M2M_CLIENT_SECRET is not currently visible at the repo-secret level and may still need to be added. If it's unset, this expression resolves to an empty string rather than erroring, so the job dispatches and then fails during omnigent auth — a red check on the PR with a non-obvious cause.

Recommend confirming the secret (repo or org level) before merging, since issue_comment workflows are read from the default branch and go live for everyone the moment this lands. Optionally add if: secrets.OMNIGENT_M2M_CLIENT_SECRET != ''-style guarding, though caller-level secrets context isn't available in job if:, so confirming the secret is the practical fix.

# (to the next release's commit SHA) to adopt a new xreview release. The verdict is posted to
# the PR as one sticky comment when a review produces one.
on:
issue_comment:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Two issue_comment properties worth noting for the test plan:

  1. Workflows triggered by issue_comment are always loaded from the default branch, so the test-plan step "open a test PR, comment seidroid xreview" will not exercise this file until after merge. Plan to validate post-merge.
  2. The trust gate keys off github.event.comment.author_association — the commenter — not the PR author or head repo. A MEMBER can therefore trigger the review on a fork PR containing untrusted head code, with the job holding pull-requests: write and the M2M secret. That's the standard shape for this pattern and presumably fine given xreview's sandbox, but the sandbox is the only thing standing between untrusted code and those credentials — worth confirming that boundary in uci rather than assuming it.

@masih masih left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good.

Blocking until UCI part is reviewed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants