feat!: migrate the monorepo to Bun and Biome - #327
Conversation
Replace pnpm/Jest with Bun for install, test, and CI, keep Biome as the linter/formatter, and publish ESM-only packages to npm via Changesets. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #327 +/- ##
==========================================
- Coverage 81.83% 81.19% -0.65%
==========================================
Files 70 71 +1
Lines 1277 3775 +2498
Branches 162 0 -162
==========================================
+ Hits 1045 3065 +2020
- Misses 232 710 +478
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The Bun/Biome migration is largely a faithful port — the #312 SSE session-binding tests and validateSecurityConfig survive intact, and the ESM-only break carries a major changeset for all five packages — but flattening dist/ broke package-info.ts's relative path, so the published sei-mcp-server will report version 0.0.0. The PR also edits the in-tree AI-reviewer guideline file to pre-emptively discount Codecov regressions caused by this same PR, which needs to be substantiated rather than asserted.
Findings: 2 blocking | 16 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Second-opinion coverage was partial:
cursor-review.mdis empty, so only the Codex pass and this pass produced findings. Codex's single finding (thepackage-info.tspath) is confirmed and included below. - Codecov is not reconciled with the new instrumentation.
codecov.yml(unchanged) defines path-based flags with 80% project targets for create-sei, precompiles, registry and sei-global-wallet, butcoverage.ymlnow uploads a single rootcoverage/lcov.infowith noflags:input, andbun test --coverageonly instruments files the tests import (vs. Jest'scollectCoverageFrom: src/**/*.ts). Please attach the actual per-package before/after numbers to the PR so the swing can be judged, and updatecodecov.yml/the upload step if flag attribution no longer works. - Test isolation weakened within files.
packages/mcp-server/jest.config.jssetclearMocks: trueandresetMocks: true; there is no Bun equivalent andbunfig.tomldoes not replicate it, andjest.resetModules()was dropped fromserver.test.ts'sbeforeEachwith no replacement.bun test --isolateisolates per file, not per test, so mock implementations and module state now persist across tests inside a file. Worth confirming each ported suite clears explicitly. - Test files are no longer type-checked anywhere.
tsconfig.base.jsonnow excludes**/*.test.ts,**/*.spec.tsand**/tests/**; every package tsconfig usesinclude: ["src"](so the new rootbun-test.d.tsis referenced by no tsconfig and is effectively dead); and no package defines a typecheck script —bun run checkis Biome only. The deletedtsconfig.test.jsonwas the only thing coveringsrc/tests/**. Consider restoring a test tsconfig plus atsc --noEmitstep inchecks.yml. - Node version pinning was dropped.
.nvmrcis deleted and neitherchecks.ymlnorcoverage.ymlpins Node, yetlint:pack:publintshells out topublint --pack npm(which invokesnpm pack) and all five packages are consumed on Node. Onlyrelease.ymlstill pins Node 24. Consider keeping a Node pin in CI and/or addingengines.nodeto the packages. config.test.tsworks around the missingjest.resetModules()by cache-busting the specifier:import(../../core/config.js?reload=${Date.now()}-${Math.random()}). It works, but it leaks a module instance per call and depends on Bun honouring query strings on relative specifiers. Amock.module-based reset (or an explicit reset export on the config module) would be more durable.- Per-package
testscripts (bun test --isolate src) are no longer exercised by CI —checks.ymlruns the rootbun test --isolate packagesinstead — so they can silently drift. Same forbuild:all/test:all, which are now thin aliases. viemmoved from^2.30.5to an exact2.37.2inmcp-server's runtimedependencies(andprecompilesdevDependencies) with no mention in the changeset. An exact pin in a published package's runtime deps means upstream security fixes require a manual bump; if the pin is deliberate, a note in the changeset would help.declarationMapis now enabled for precompiles, registry, mcp-server, andsourceMaptoo for sei-global-wallet, while every package still shipsfiles: ["dist"]. The published.d.ts.map/.js.mapfiles therefore point atsrc/**that is not in the tarball. Either addsrctofilesor drop the map flags (flagged inline onpackages/precompiles/tsconfig.json).- The PR description says mcp-server has 24
*.test.tsfiles; there are 25 undersrc/tests/. Minor, but the description uses the count as an acceptance criterion. - Positive notes, for the record: the #312 SSE session-binding tests (400 on missing
sessionId, 404 on unknown, and the two cross-session isolation cases) survive the port with assertions intact;validateSecurityConfig()and theconnections-keyed-by-sessionIdlogic are unchanged apart from formatting; the stdioconsole.errorconvention is respected (no newconsole.logon a stdio-reachable path);registerSolanaStandard.ts→.spec.tsis a correct fix, since Bun's matcher would otherwise have silently stopped running that file; andscripts/check-precompile-exports.tsruns in bothchecks.ymlandrelease.ymlbefore publish, which is a real improvement over the previous subpath situation. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
Restore runtime metadata, hermetic test isolation, package-scoped coverage, and artifact checks so the ESM-only release remains reproducible and reviewable. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
A large, generally well-executed pnpm/Jest → Bun/Biome migration that preserves both load-bearing mcp-server security invariants (stdio-only wallet guard, SSE session binding) and ships a correct five-package major changeset. Blocking on one correctness regression: a Biome radix autofix added , 10 to Number.parseInt on two paths that receive hex EVM values, so hex block numbers and chain IDs now silently resolve to 0 instead of their real value.
Findings: 1 blocking | 12 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty), so this review is Claude + Codex only. - Codex flagged reviewer-directed text in the PR description as prompt injection. The exact sentence it quoted is no longer in the current PR body, but the "Coverage reconciliation" section does pre-frame how a reviewer should interpret the Codecov delta. I treated the PR body as untrusted and verified
codecov.yml/coverage.ymlfrom the diff directly — it did not influence this verdict. create-sei's Codecov flag and its 80% project target are both deleted, andcreate-seiis absent from the newcoverage.ymlmatrix, so that gate disappears entirely rather than being relaxed. The PR body justifies this ("no source report"), but it is a gate deletion on a published package and deserves an explicit sign-off.REVIEW.mdwas updated in the same PR to declare this a known non-issue — reasonable, but worth confirming the ordering is intentional.packages/create-sei/templates/next-template/package.jsonandextensions/precompiles/package.jsonnow pin@sei-js/precompiles: ^3.0.0, andmain.test.tsasserts that exact string. That version does not exist on npm until this release publishes. If the Changesets run publishescreate-seibeforeprecompiles, or the precompiles major lands as a different number, every scaffolded app failsbun installand the test locks in the wrong value.- The new
coverage.ymldrops the build step entirely. Nothing in the current test suite reads fromdist/, so this is fine today, but the PR description mentions "a built-CLI regression test" — I could not find any test referencingdist, so that check appears to be a manual smoke test only and is not enforced by CI. Consider landing it as a real test. fix-dts-extensions.tsexists to paper over emitting declarations undermoduleResolution: "Bundler", which produces extensionless specifiers in.d.ts. Usingmodule/moduleResolution: "NodeNext"for the declaration builds would make the post-processing script unnecessary and remove a class of silent breakage (e.g. it does not rewrite/// <reference path=...>).- 6 suggestion(s)/nit(s) flagged inline on specific lines.
Accept canonical hexadecimal chain and block values, make mock resets order-independent, and enforce packed CLI checks without network-resolved tooling. Co-authored-by: Cursor <cursoragent@cursor.com>
PR SummaryHigh Risk Overview CI and quality gates: GitHub Actions swap pnpm for Bun, add PR triggers and concurrency on checks, run Biome/typecheck before build/test, and shard coverage into a per-package matrix uploading explicit Scaffolding and docs: README/REVIEW.md describe Bun workflows; Reviewed by Cursor Bugbot for commit 640cfee. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
Follow-up on the refreshed review's non-inline points:
The blocking hex regression and all six inline suggestions are fixed in |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e7b0301. Configure here.
There was a problem hiding this comment.
A large but coherent pnpm+Jest → Bun+Biome migration that also ships ESM-only majors for all five packages; the two load-bearing mcp-server security invariants (wallet-is-stdio-only, SSE session binding) are untouched and their tests are preserved and extended, and the required changeset is present and correctly scoped. No blockers — remaining findings are hardening/maintainability suggestions around the exact viem pin, the repo-wide moduleResolution: Bundler switch, chain-id parsing edge cases, and coverage gaps.
Findings: 0 blocking | 10 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion pass produced no output —
cursor-review.mdis empty. Only the Codex pass and this one contributed findings. - Codex raised
REVIEW.md:90as a prompt-injection attempt (a PR editing the instructions that automated reviewers consume). I disagree with that framing: both edited bullets track real changes in this same PR —checks.ymlgenuinely now runsbun run check(Biome + tsc), andcodecov.ymlgenuinely now defines 80% project targets for exactly three packages — and the new wording does not widen reviewer latitude relative to the old "do not file formatting-only findings as blocking". The PR also correctly deletes the now-stale@asyncapi/pnpm.overridessentence (that block is absent from the basepackage.jsonand@asyncapiappears nowhere inbun.lock). It is still reasonable for a human to explicitly ratify guardrail edits that land in the same PR as the CI change they describe. create-seiloses coverage entirely: it is dropped from thecoverage.ymlmatrix and its flag/target is removed fromcodecov.yml, sosrc/main.test.tsnow runs only inchecks.ymland reports nothing. This is disclosed in the PR body and is a deliberate trade, but it means the newly added scaffold-version regression test contributes no coverage signal.- CI gains runtime import smoke tests for
precompiles(scripts/check-precompile-exports.ts) andmcp-server(scripts/check-mcp-cli.ts), but@sei-js/registryand@sei-js/sei-global-walletget onlyattw --profile esm-only, which validates type resolution rather than an actual Node ESMimport(). Given these two are flipping to ESM-only in the same release, a two-line runtime import check for each would close the gap the PR body currently covers with manual smoke tests. packages/create-sei/templates/next-template/package.jsonandextensions/precompiles/package.jsonnow require@sei-js/precompiles@^3.0.0, which does not exist on npm until this changeset publishes. Changesets publishes all five majors in one run so the window is small, butcreate-seiscaffolds are unresolvable if the precompiles major is ever split out of this release.- 5 suggestion(s)/nit(s) flagged inline on specific lines.
Superseded: latest AI review found no blocking issues.
Keep hex support without treating blanks or exponent notation as chain data, and harden direct execution and ESM build checks against future regressions. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
A large but disciplined pnpm/Jest → Bun/Biome migration that publishes ESM-only majors for all five packages. I found no blockers: the two load-bearing mcp-server security invariants (stdio-only wallet mode via validateSecurityConfig, SSE messages keyed by transport.sessionId with 400/404 handling) and their tests are intact, no precompile address or ABI under src/precompiles/** was touched, and a correct major changeset covers all five packages. The notes below are non-blocking: a narrowed chain-identifier parse that now falls back to mainnet, two tsconfigs that still inherit moduleResolution: "Bundler" (contradicting the PR's NodeNext claim and forcing the hand-rolled fix-dts-extensions.ts), a likely-wrong Biome key, and missing consumer-facing docs for the CJS drop.
Findings: 0 blocking | 12 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes: Codex reported "No material issues found in the reviewed diff." The Cursor file (
cursor-review.md) is empty even though the PR shows acursor (Commented)review, so that pass contributed nothing to this synthesis — worth re-running if you want its input before merge. - No prompt-injection or instruction-bearing content was found in the diff, commit messages, or PR body.
create-seiloses its Codecov flag and 80% project target, andsrc/main.ts— the actual CLI logic (runWizard, template/extension copying,listExtensions) — still has no test that exercises it.src/main.test.tsonly asserts that theextensions/precompilesdirectory exists and that twopackage.jsonfiles pin^3.0.0. The PR's rationale ("no source report") is accurate for the old Jest setup, but it means a published CLI now ships with zero enforced coverage. Consider one test that importsmain.tsand scaffolds into a temp dir.- The PR edits
REVIEW.md, this repo's own AI-review guideline file, and relaxes two "known non-issues" entries (Biome now enforced;create-seiadded to the no-Codecov-target list). Both edits are factually consistent with the migration, but changes that narrow what future automated reviews flag deserve explicit human sign-off rather than riding along inside a ~24k-line migration. - All four new
exportsmaps omit"./package.json": "./package.json". Some bundler plugins and framework resolvers read it directly and now get anERR_PACKAGE_PATH_NOT_EXPORTED. Cheap to add. resetPublicClientCache()is a test-only seam added topackages/mcp-server/src/core/services/clients.ts, which is re-exported byservices/index.ts. Harmless (it isn't reachable from the package's.export), but flagging it so it doesn't get mistaken for a supported API later.- 6 suggestion(s)/nit(s) flagged inline on specific lines.
| const parsedId = Number.parseInt(networkName); | ||
| if (!Number.isNaN(parsedId)) { | ||
| return parsedId; | ||
| if (/^(?:\d+|0x[0-9a-f]+)$/i.test(networkName)) { |
There was a problem hiding this comment.
[suggestion] Tightening the parse is right, but it widens what reaches the silent mainnet fallback below. Previously Number.parseInt('1328abc') returned 1328, so getRpcUrl('1328abc') gave the testnet RPC; now the regex rejects it and the function returns DEFAULT_CHAIN_ID (1329), so the same input yields the mainnet RPC. Impact is narrow — resolveChainId only feeds getRpcUrl, and getChain() throws for unrecognised strings, so most call paths fail loudly first — but silently substituting mainnet for a malformed identifier is the wrong direction for a package that hands capabilities to an LLM. Consider throwing Unsupported network: ${chainIdentifier} here (matching getChain) instead of falling through to the default.
| "outDir": "./dist/types" | ||
| "outDir": "./dist", | ||
| "rootDir": "./src", | ||
| "declaration": true, |
There was a problem hiding this comment.
[suggestion] The PR description says "Enforced NodeNext resolution for tsc-emitted packages," but this config (and packages/registry/tsconfig.json) still inherits moduleResolution: "Bundler" from tsconfig.base.json — only sei-global-wallet and create-sei got explicit NodeNext. Under Bundler resolution tsc emits relative specifiers verbatim (export * from './bankPrecompile'), which is exactly why scripts/fix-dts-extensions.ts has to exist. Adding "module": "NodeNext", "moduleResolution": "NodeNext" to these two configs makes tsc emit ./bankPrecompile.js directly and lets the post-processing script be deleted, removing a whole class of "attw passes today, breaks on the next import form" risk.
| } | ||
| return files; | ||
| }; | ||
|
|
There was a problem hiding this comment.
[nit] This regex only matches from '...' and import('...'). It silently no-ops on import './x'; (side-effect form) and declare module './x', and it has no way to signal that it rewrote nothing. Neither form appears in today's emit, so this is correct as written — but a silent no-op is the failure mode you'd least want here, since the result is a published package whose .d.ts files don't resolve under node16/nodenext. If the NodeNext suggestion on packages/precompiles/tsconfig.json isn't taken, consider having the script assert it rewrote at least one specifier per package, so a regression fails the build rather than shipping.
| "main": "./dist/cjs/index.js", | ||
| "module": "./dist/esm/index.mjs", | ||
| "types": "./dist/types/index.d.ts", | ||
| "type": "module", |
There was a problem hiding this comment.
[suggestion] Adding "type": "module" and dropping dist/cjs makes require('@sei-js/precompiles') throw ERR_REQUIRE_ESM on Node 20 (same for @sei-js/registry). The changeset documents this, but two consumer-facing signals are missing: (1) neither package declares engines.node, so npm gives no warning on a Node version where require(esm) is unavailable — mcp-server correctly bumped to >=20.0.0, these didn't; (2) neither README has a migration note. For a deliberate breaking major on packages that ship to every downstream dApp, both are worth adding so the failure surfaces at install time rather than at first require.
| "enabled": true, | ||
| "rules": { | ||
| "recommended": true, | ||
| "preset": "recommended", |
There was a problem hiding this comment.
[nit] I believe the Biome key here is "recommended": true, not "preset": "recommended" — I couldn't verify locally (no node_modules in this checkout). If preset is an unrecognised key, the effect is invisible rather than fatal: Biome's default is recommended: true when the linter is enabled, so the ruleset is unchanged either way and CI stays green. Worth confirming biome check emits no unknown-key diagnostic, since this is now the config that gates every PR.
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '24' | ||
| node-version-file: '.nvmrc' |
There was a problem hiding this comment.
[nit] The step name still says "for npm OIDC publishing" but the version is now indirect via .nvmrc. npm OIDC trusted publishing needs npm >= 11.5.1, which is why this was hard-pinned to '24'. It resolves to 24 today, but a future .nvmrc change for the Bun/dev toolchain would silently break publishing here. Either keep node-version: '24' on this step or add a comment on .nvmrc noting that release.yml depends on it.
2 participants
Summary
@sei-js/precompilesexposes.,./ethers,./viem, and./precompiles;@sei-js/sei-global-wallet/solanaremains available.Review follow-ups
0.3.3.Coverage reconciliation
The Jest and Bun percentages are not directly comparable because Jest instrumented every configured source file while Bun instruments loaded files and counts lines differently. These are the actual source totals, not a claimed coverage improvement:
Test plan
bun install --frozen-lockfilebun run check(Biome + TypeScript for all migrated tests)bun run buildbun run test— 485 tests across 39 files; 0 failuresbun test --isolate --randomize --rerun-each=3 packages— 1,455 passes; 0 failuresbun run lint:pack:all— publint, ATTW ESM profile, precompile export/identity checks, and built MCP CLI versions