Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions .github/workflows/ai-assist.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
name: AI Assistant
on:
issue_comment:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[nit] issue_comment fires for comments on issues as well as PRs, from any user on a public repo, so every drive-by comment anywhere in the repo spawns a workflow run. Gating rests entirely on UCI's allowed-team check (line 22).

Delegating the authorization check to the reusable workflow is a reasonable design, but worth verifying two things at the pinned SHA: that the team check runs before any comment body reaches the agent as instructions, and that a non-member comment exits cleanly rather than as a failed run.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[suggestion] issue_comment: created with no if: guard means a job starts for every comment on every issue and PR in the repo, then presumably exits inside UCI once it finds no @seidroid mention or a non-sei-core author.

Two consequences worth a caller-side guard:

  1. Runner minutes and a noisy Actions tab on ordinary discussion.
  2. Feedback-loop risk: UCI posts as a GitHub App with a PLATFORM_CODE_AGENT_* token, not GITHUB_TOKEN, so its own comments and review submissions do re-trigger workflows. Combined with pull_request_review: submitted, the AI Review workflow posting a review can wake the assistant.

The allowed-team check likely stops the loop (an App isn't a team member), but relying on that means the loop is only broken inside a pinned third-party workflow. A cheap explicit guard:

    if: >-
      github.event.sender.type != 'Bot' &&
      contains(github.event.comment.body || github.event.review.body, '@seidroid')

A concurrency group keyed on the issue/PR number would also be worth adding here.

types: [ created ]
pull_request_review_comment:
types: [ created ]
pull_request_review:
types: [ submitted ]
jobs:
assistant:
# See: https://github.com/sei-protocol/uci/releases/tag/v0.0.15
uses: sei-protocol/uci/.github/workflows/ai-assistant.yml@65901242783550521f25a19199a6b10e54550b97
permissions:
contents: read

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[nit] Worth confirming contents: read is sufficient at the pinned SHA. If UCI's assistant can apply fixes — commit to the PR branch or push a suggestion branch in response to @seidroid — it needs contents: write, and this would fail only at the point someone first asks it to make a change. I couldn't check the v0.0.15 definition from here (no network), so flagging rather than asserting.

pull-requests: write
issues: write
id-token: write
secrets: inherit
with:
# See: https://github.com/sei-protocol/uci/releases/tag/v0.0.15
uci-ref: 65901242783550521f25a19199a6b10e54550b97
allowed-team: 'sei-protocol/sei-core'
20 changes: 20 additions & 0 deletions .github/workflows/ai-review.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
name: AI Review
on:
pull_request:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[suggestion] Because this uses pull_request (not pull_request_target), PRs from forks on a public repo get a read-only GITHUB_TOKEN and no secretssecrets: inherit resolves to nothing. So AI Review will fail or silently no-op on every external-contributor PR, leaving a permanently red "AI Review" check on exactly the PRs where outside contributors see it.

To be clear: staying on pull_request is the correct security call here — pull_request_target would run privileged with untrusted code checked out. The suggestion is only to make the failure non-noisy. The existing pr-to-slack-codex.yml guards on github.event.pull_request.author_association (line 10) for precisely this reason; a similar if: on this job, or a note in the PR-workflow docs, would avoid the confusing red check.

# `labeled`/`unlabeled` let the `ai: skip-review` label take effect immediately rather
# than on the next push. UCI ignores those two events for every other label.
types: [ opened, ready_for_review, synchronize, reopened, labeled, unlabeled ]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[suggestion] No concurrency guard on this caller, while uci-stale-check.yml does define one. synchronize fires on every push, and with labeled/unlabeled now in the trigger list there are more ways to stack runs — rapid pushes, or a push landing just after a label change, can run overlapping reviews on the same PR. That means duplicate posted reviews, racing check-run updates, and doubled LLM spend.

Unless UCI's ai-review.yml sets concurrency internally (worth confirming at the pinned SHA), consider:

concurrency:
  group: ai-review-${{ github.event.pull_request.number }}
  cancel-in-progress: true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[suggestion] No concurrency block on this caller, unlike uci-stale-check.yml.

With synchronize in the trigger list, every push to an open PR starts a fresh full review. During a rebase-and-force-push or a rapid series of fixup commits that means several concurrent (or queued) review runs against the same PR, each burning model tokens and each racing to post a review. Adding the same pattern already used in release.yml fixes it:

concurrency:
  group: ${{ github.workflow }}-${{ github.event.pull_request.number }}
  cancel-in-progress: true

Separately, worth being aware that pull_request (correctly chosen over pull_request_target) means fork PRs get no secrets and a read-only token — so AI review will not run on external contributions to this public repo. That's the safe tradeoff, just make sure nobody reads a missing review on a fork PR as an approval.

jobs:
ai-review:
# See: https://github.com/sei-protocol/uci/releases/tag/v0.0.15
uses: sei-protocol/uci/.github/workflows/ai-review.yml@65901242783550521f25a19199a6b10e54550b97
permissions:
contents: read
pull-requests: write
checks: write
id-token: write
secrets: inherit

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[nit] secrets: inherit forwards every repo and org secret to the reusable workflow. This repo holds OPENAI_API_KEY, SLACK_BOT_TOKEN, SLACK_CHANNEL_ID, and CODECOV_TOKEN, yet the PR description says only the PLATFORM_CODE_AGENT_* set is actually needed.

The SHA pin and same-org ownership make this acceptable, so this is hygiene rather than a vulnerability — but enumerating just the required secrets under secrets: would bound the blast radius if the pin were ever bumped to a bad commit, and would document the actual dependency. Same applies to ai-assist.yml.

with:
# See: https://github.com/sei-protocol/uci/releases/tag/v0.0.15
uci-ref: 65901242783550521f25a19199a6b10e54550b97
enable-cursor: false # Disabled for now since there is a dedicated Bugbot flow built into Cursor currently enabled on repo.
28 changes: 28 additions & 0 deletions .github/workflows/uci-stale-check.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
name: UCI
run-name: UCI / Stale Check

on:
workflow_dispatch:
schedule:
- cron: '0 12 * * *'

permissions:
contents: read

concurrency:
group: ${{ github.workflow }}-${{ github.sha }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[nit] Keying concurrency on github.sha is an odd fit for a scheduled workflow: for cron runs the SHA is the tip of the default branch, so it stays constant between commits. A workflow_dispatch run will land in the same group as an in-flight scheduled run on the same SHA and, with cancel-in-progress: true, cancel it (and vice versa).

github.ref or a plain constant is the more usual key here. Separately, github.workflow resolves to UCI (line 1), which is generic enough to collide if another workflow later adopts that same name:.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[nit] Keying the concurrency group on github.sha is a slightly odd fit for a cron/dispatch workflow. Consecutive daily runs on an unchanged default branch share the same sha and so land in the same group, while a workflow_dispatch fired during a running cron will cancel it. ${{ github.workflow }}-${{ github.ref }} — the pattern release.yml:8 already uses — expresses "one stale sweep at a time per branch" more directly.

cancel-in-progress: true

jobs:
stale:
name: Stale
# Job-level `permissions` replaces the workflow-level block rather than merging,
# so `contents: read` has to be repeated here to survive.
permissions:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[suggestion] Job-level permissions replaces the workflow-level block rather than merging with it, so any scope not listed here is set to none. This job therefore runs with contents: none, and the workflow-level contents: read on line 10 applies to no job at all (stale is the only one).

Both sibling workflows added in this PR explicitly include contents: read in their job-level block, which suggests the omission here is accidental rather than deliberate. It's likely harmless if UCI's stale-check.yml only calls the issues/PRs API, but it breaks the moment that workflow does an actions/checkout.

Suggest adding contents: read to this block (and dropping the now-dead top-level permissions: on lines 9-10, or keeping it only as documentation).

contents: read
issues: write

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[suggestion] This job is granted issues: write on a daily cron, but the only input passed is days-before-pr-stale: 28 — issue staling is left entirely to whatever UCI v0.0.15 defaults to.

Since this runs unattended every day at 12:00 UTC and can comment on or close issues, the issue-side behaviour should be explicit rather than inherited: either pass days-before-issue-stale (and the corresponding close/exempt inputs) deliberately, or, if issues aren't meant to be triaged at all yet, drop issues: write so an upstream default change can't start closing them. Worth confirming against the v0.0.15 input defaults before merge.

pull-requests: write
# See: https://github.com/sei-protocol/uci/releases/tag/v0.0.15
uses: sei-protocol/uci/.github/workflows/stale-check.yml@65901242783550521f25a19199a6b10e54550b97
with:
days-before-pr-stale: 28
109 changes: 109 additions & 0 deletions REVIEW.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
# Review guidelines for AI agents

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[blocker] The filename looks wrong, which would make this whole file dead weight.

The PR description says this "is the guidelines file UCI's reviewer loads from the PR base branch" — but the reviewer harness that ran on this PR was handed a base-branch file named REVIEW_GUIDELINES.md, not REVIEW.md. Those two names can't both be right.

Worth checking directly against sei-protocol/uci/.github/workflows/ai-review.yml@65901242 which path it fetches from the base ref. If it's REVIEW_GUIDELINES.md, rename this file — otherwise the reviewer sees no guidelines and every §4 "known non-issue" keeps getting filed as a finding, which is the main thing this file exists to prevent.

(If v0.0.15 genuinely reads REVIEW.md and a later UCI renamed it, then this is fine as-is and the note is on the uci-ref pin instead.)


Repo-specific conventions for automated PR review (Codex, Cursor, Claude, and
any other AI reviewer). This is a pnpm workspace that publishes six
independently versioned `@sei-js/*` packages to npm, so a defect ships to every
downstream dApp that upgrades rather than to a single deployment we control.
Calibrate accordingly: a wrong precompile address or a weakened wallet guard is
far more valuable to catch than a style nit, and several patterns below look
like bugs in isolation but are deliberate.

## 1. `packages/mcp-server` is the security surface

This package hands blockchain capabilities to an LLM client, so it is the one
place in the repo where a subtle regression can cost users funds. Two
invariants are load-bearing and are enforced in code rather than by convention:

- **Wallet mode is stdio-only.** `validateSecurityConfig()` in
`src/server/transport/security.ts` halts the process when the wallet is
enabled and the transport is `streamable-http` or `http-sse`. HTTP
transports are reachable cross-origin, so a signing key behind one is a
drain-the-wallet primitive. Any change that narrows this check, makes it
non-fatal, or adds a transport that bypasses it is a finding.
- **SSE messages are bound to their session.**
`src/server/transport/http-sse.ts` keys `connections` by
`transport.sessionId` and requires a matching `?sessionId=` on
`POST {path}/message` (400 when absent, 404 when unknown). This replaced an
implementation that routed to the first connection in the map, which let one
client inject into another's stream. Treat any reintroduction of positional
or implicit session lookup as a regression, and keep the isolation tests in
`src/tests/server/transport/http-sse.test.ts` meaningful.

Beyond those, scrutinise anything that widens what a caller controls: contract
ABIs reach `JSON.parse` from tool arguments in `src/core/tools.ts`, addresses
and call arguments arrive unvalidated from the model, and RPC endpoints are
overridable through `MAINNET_RPC_URL` / `TESTNET_RPC_URL` / `DEVNET_RPC_URL` in
`src/core/chains.ts`. Private keys are read from the environment in
`src/core/config.ts` and must never reach a log line, an error message, or a
tool response.

Note that `.github/workflows/pr-to-slack-codex.yml` already runs a separate
AppSec pass to Slack. Overlapping findings are expected; don't suppress a real
issue because you assume the other reviewer caught it.

## 2. Precompile addresses and ABIs are hand-maintained source

`packages/precompiles/src/precompiles/*.ts` is not generated — there is no
codegen step or ABI pipeline in this repo. An address or ABI entry changed
there is a change to the source of truth, and a wrong value silently misroutes
every consumer's calls. You usually cannot settle these from the diff alone, so
when a value looks suspect, ask for the authoritative source rather than
asserting it is wrong: link what you checked (`docs.sei.io`, `sei-chain`,
Seiscan) and say what disagrees. An unsourced change to an existing documented
address, chain ID, or ABI signature is worth raising on its own.

## 3. A publishable change needs a changeset

`.changeset/config.json` sets `fixed: []` and `linked: []`, so every package
versions independently — do not expect or request a coordinated bump. Merging
to `main` opens a "Version Packages" PR, and merging that publishes. A
user-facing change to a published package with no `.changeset/*.md` file ships
the code without releasing it, which is the common miss on this repo.

Ask for a changeset when a published package's behaviour, types, or
dependencies change. Docs-only, CI-only, and changes confined to
`packages/create-sei/templates/**` generally don't need one, though the repo
has deliberately added patch changesets across all six packages for
release-note visibility (the `@asyncapi` pinning in `pnpm.overrides` is the
precedent). Absence of a changeset on that kind of PR is a question, not a
defect.

## 4. Known non-issues — do not flag these

- **`console.error` used for informational messages.** Under the stdio
transport, stdout carries the JSON-RPC frames, so anything written there
corrupts the protocol. `console.error('MCP Server ready (stdio transport)')`
in `src/server/transport/stdio.ts` is correct. Never suggest converting
these to `console.log`. The inverse *is* a finding: a new `console.log` on a
path reachable from stdio breaks the transport.
- **The CORS middleware sets no `Access-Control-Allow-Origin`.**

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[suggestion] This entry is accurate about browsers but risks suppressing a real gap.

"Deny-by-default" only holds for browser clients that honour the missing Access-Control-Allow-Origin. Server-side there is no Origin or Host validation anywhere in http-sse.ts / streamable-http.ts, so a non-browser client — or a DNS-rebinding attack against a locally bound server — reaches the tool surface unimpeded. The MCP spec calls for explicit Origin validation on local HTTP transports for exactly this reason.

The wallet guard in validateSecurityConfig() keeps funds out of reach, so this isn't a drain primitive, but it still exposes the read tools and the configured RPC endpoint. Suggest narrowing the wording to "the absence of a permissive wildcard is deliberate — missing Origin/Host validation is a separate question and is still fair to raise", so a future reviewer isn't told to stay quiet about it.

`createCorsMiddleware()` answers preflights with a bare 204 and no CORS
headers. That is deny-by-default: a browser treats the missing header as a
failure and blocks the request. It is not an oversight and not a permissive
wildcard.
- **`process.exit(1)` inside `validateSecurityConfig()`.** Failing closed at
startup is the intent. Do not ask for a thrown error the caller might swallow.
- **`packages/registry/chain-registry` and `.../community-assetlist` are
missing from the tree.** Both are git submodules (`.gitmodules`) and are
listed in `.gitignore`; they are populated by the `registry` package's
`postinstall` and by CI's submodule checkout. Their JSON is vendored

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[nit] "and by CI's submodule checkout" is only half true: release.yml:24 sets submodules: recursive, but checks.yml — the workflow that actually gates PRs — does a plain actions/checkout@v4 and relies solely on the registry package's postinstall. Since a reviewer reading this line is most likely looking at a PR-gate failure, naming release.yml specifically would avoid pointing them at the wrong mechanism.

upstream — review the TypeScript wrappers, not the data.
- **Biome findings are not enforced anywhere.** `biome.json` configures tabs,
160-column lines, single quotes and no trailing commas, but no package
defines a `biome` script and `.github/workflows/checks.yml` runs only
`pnpm build:all` and `pnpm test:all`. Match the surrounding style; do not
file formatting-only findings as blocking.
- **Test file naming is inconsistent across packages.** `mcp-server` and
`create-sei` use `*.test.ts`; `precompiles`, `ledger`, `registry` and
`sei-global-wallet` use `*.spec.ts` under `__tests__/`. Follow the
convention of the package being changed rather than proposing a repo-wide
rename.
- **`mcp-server` has no Codecov target.** `codecov.yml` defines 80% project
targets for the other five packages only. Thin coverage on an mcp-server PR
is worth mentioning on its merits, but it does not fail a gate.
- **`noImplicitAny: false` in `tsconfig.base.json`.** This is a deliberate
repo-wide setting. Flag an untyped value when it actually causes an unsound
path, not because the compiler permitted it.
- **The `create-sei` templates are excluded from the root Biome config** and
carry their own toolchain. Do not apply root formatting rules to anything
under `packages/create-sei/templates/`.
Loading