fix: standardize package publishing hygiene (PLT-847) - #321
fix: standardize package publishing hygiene (PLT-847)#321alexander-sei wants to merge 2 commits into
Conversation
Ensure every public package ships consistent licensing, clean artifacts, and verified type entrypoints. Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #321 +/- ##
=======================================
Coverage 79.75% 79.75%
=======================================
Files 83 83
Lines 1309 1309
Branches 217 217
=======================================
Hits 1044 1044
Misses 259 259
Partials 6 6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Solid publish-hygiene cleanup: the LICENSE/files/publishConfig standardization is right, the sei-global-wallet dist/cjs/package.json marker fixes a real bug (the package is "type": "module", so its CJS output was being parsed as ESM), and the mcp-server Node16 + build-config split correctly stops shipping compiled tests. No blockers found and the MCP security invariants are untouched, but a few items are worth confirming before merge — chiefly the publint run token in the new CI script and the fact that the new .mjs ESM entrypoints are still unreachable from Node without an exports map.
Findings: 0 blocking | 15 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty), so this review merges only my findings with Codex's. - Please confirm the new Validate Published Packages CI step actually goes green on this branch — it is the PR's headline feature and this is the first run of
publint/attwin this repo, so any CLI-surface mismatch shows up there first. lint:pack:allis a single&&chain, so the first failing tool hides the other two. Splitting it intolint:pack:publint/lint:pack:attw(or adding--no-bailto the recursiveexec) would surface every packaging problem in one CI run instead of one per push.- The
Validate Published Packagesstep is placed beforeTest, so a packaging failure masks the test results. Running it afterTest(or as a separate job) gives better signal per run. esbuildis invoked byledger,precompiles, andregistrybuild scripts but declared only in the rootdevDependencies. It resolves via the workspace-rootnode_modules/.binon PATH — the same way the existingrimrafusage works, so this is consistent with the repo — but declaring it in each consuming package would make those builds self-contained.- Published surface is being narrowed in ways a
patchbump doesn't advertise:files: ["dist"]is newly added tocreate-sei,precompiles, andsei-global-wallet(previouslysrc/shipped), anddist/esmforledger/precompiles/registrycollapses from a full tsc tree to one.mjs. None of these packages have anexportsmap, so paths like@sei-js/precompiles/dist/esm/viem/...and@sei-js/precompiles/src/...were reachable and now break. No in-repo consumers do this, so it's likely fine — just a deliberate call worth making explicitly. - Neither
publintnorattwchecks the two things the manual test plan verified (LICENSEpresent in the tarball, no test files indist). A smallnpm pack --dry-run --jsonassertion would lock in what this PR just fixed and stop it regressing on the next build-config change. - Adjacent publish-hygiene item this PR could close out:
packages/registrystill carriespostinstall: git submodule update --init --recursive, which runs in every consumer'snode_moduleswhere there is no git repository. Gating it (e.g. on the directory being a git checkout) would fit the stated scope. - No prompt-injection or instruction-like content was found in the diff, commit messages, or PR description.
- 6 suggestion(s)/nit(s) flagged inline on specific lines.
| "scripts": { | ||
| "build:all": "pnpm -r run build", | ||
| "docs": "cd docs && pnpm dlx mint dev", | ||
| "lint:pack:all": "pnpm --recursive --filter './packages/*' exec publint run --pack npm --strict --level warning && pnpm --recursive --filter './packages/*' --filter '!@sei-js/create-sei' --filter '!@sei-js/mcp-server' exec attw --pack . --profile node16 && pnpm --filter @sei-js/mcp-server exec attw --pack . --profile esm-only", |
There was a problem hiding this comment.
[suggestion] Codex flagged publint run as invalid, arguing run is parsed as the package-directory positional and CI would lint a nonexistent ./run. I could not confirm either way without network access to publint 0.3.23's CLI, so I'm not calling it a blocker — but run is redundant regardless, since linting the current directory is publint's default command. Dropping it (publint --pack npm --strict --level warning) removes the ambiguity at zero cost:
pnpm --recursive --filter './packages/*' exec publint --pack npm --strict --level warning && ...
If run is a valid subcommand the behaviour is identical; if it isn't, this is the difference between a green and a red pipeline.
| "description": "TypeScript library for EVM interactions on the Sei blockchain", | ||
| "main": "./dist/cjs/index.js", | ||
| "module": "./dist/esm/index.js", | ||
| "module": "./dist/esm/index.mjs", |
There was a problem hiding this comment.
[suggestion] Switching module to a real .mjs bundle is a genuine improvement for bundlers — the previous tsc --module esnext output emitted extensionless relative imports, which Node ESM cannot resolve. But module is a bundler-only convention that Node ignores, and this package (like ledger and registry) has no exports map. So import { ... } from '@sei-js/precompiles' under Node still falls back to main and lands on the CJS build; the new .mjs is unreachable from Node.
If fixing ESM entrypoints for Node consumers is part of the goal, these three packages need an exports map alongside the .mjs:
"exports": {
".": {
"import": { "types": "./dist/types/index.d.ts", "default": "./dist/esm/index.mjs" },
"require": { "types": "./dist/types/index.d.ts", "default": "./dist/cjs/index.js" }
}
}(Note the require.types would need the same types-cjs treatment sei-global-wallet just got, or a second declaration emit.) Worth checking what attw --profile node16 reports for these three — it should currently classify them as CJS-only.
| "types": "dist/types/index.d.ts", | ||
| "typings": "./dist/types/index.d.ts", | ||
| "license": "ISC", | ||
| "license": "MIT", |
There was a problem hiding this comment.
[suggestion] This relicenses the package from ISC to MIT. Both are permissive and this aligns the package with the rest of the repo, so it's very likely the intent — but it's a legal change shipping under a patch changeset whose note only mentions "licensing". Worth an explicit confirmation that the relicense is approved and that prior contributions can be relicensed, and worth calling out by name in the changeset body so it lands in the release notes rather than being inferred from a diff.
| "outDir": "./dist/esm", | ||
| "moduleResolution": "node", | ||
| "module": "ES2020", | ||
| "moduleResolution": "Node16", |
There was a problem hiding this comment.
[nit] The Node16 switch is safe for the build and for jest — tsconfig.build.json excludes src/tests/**, and tsconfig.test.json overrides back to module: ES2020 / moduleResolution: Bundler. But this file keeps include: ["src"], so the default project still covers the tests under Node16 semantics, where their extensionless directory imports are invalid:
src/tests/core/services/balance.test.ts:2 from '../../../core/services'
src/tests/core/services/transfer.test.ts:2 from '../../../core/services'
src/tests/core/services/contracts.test.ts:6 from '../../../core/services'
Nothing in CI runs a bare tsc --noEmit, so this won't fail the gate — it'll just show as red squiggles in editors, which tends to get worked around rather than fixed. Adding the same exclude list to this file (or having editors pick up tsconfig.build.json) keeps the default project clean.
| "build": "rimraf dist && pnpm run build:cjs && pnpm run build:esm && pnpm run build:types", | ||
| "build:cjs": "tsc --outDir dist/cjs --module commonjs", | ||
| "build:esm": "tsc --outDir dist/esm --module esnext", | ||
| "build:esm": "esbuild src/index.ts --bundle --format=esm --platform=neutral --packages=external --target=es2020 --outfile=dist/esm/src/index.mjs", |
There was a problem hiding this comment.
[nit] Bundling with esbuild inlines all six submodule JSON files (chain_info, chains, gas, ibc_info, wallets, assetlist) directly into dist/esm/src/index.mjs. That's arguably a fix — the old tsc ESM output emitted bare import ... from '../../chain-registry/chain_info.json', which Node ESM rejects without an import ... with { type: 'json' } attribute — but it does mean the ESM entry now carries the full registry payload as JS source while the CJS build still loads it from separate .json files. Worth a quick look at the resulting dist/esm/src/index.mjs size so the bundler-facing entrypoint doesn't grow unexpectedly for consumers who only import CHAIN_IDS.
| const commonJsTypesDirectory = join(packageRoot, 'dist/types-cjs'); | ||
|
|
||
| await writeFile(join(packageRoot, 'dist/cjs/package.json'), '{"type":"commonjs"}\n'); | ||
| await cp(typesDirectory, commonJsTypesDirectory, { recursive: true }); |
There was a problem hiding this comment.
[nit] This duplicates the entire declaration tree into dist/types-cjs purely to attach a {"type":"commonjs"} marker, doubling the published .d.ts payload. It's the standard workaround and it's correct, so no objection — but since the two trees are byte-identical, a dist/types/cjs/package.json + re-export shim, or emitting the CJS declarations from a second tsc project, would avoid the duplication. Either way, please confirm attw --profile node16 is clean for this package: the new require branch now advertises CJS types for export * from '@dynamic-labs/global-wallet-client/features', and if that dependency is ESM-only, attw should surface it.
Retain the fork's upstream licensing and align the package metadata, badge, and shipped license text. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Solid, well-scoped packaging-hygiene PR: I verified that every relative import in mcp-server/src and sei-global-wallet/src is already extension-qualified (so the Node16 moduleResolution switch and the .js-suffix additions are complete, not partial), that tsconfig.build.json genuinely drops the previously-published src/tests/core/helpers/tool-test-helpers.ts and create-sei's dist/main.test.js, and that the .mjs rename fixes ESM files that were previously mislabeled inside implicitly-CommonJS packages. No blockers; the notes below are follow-ups — the largest being whether patch is the right changeset bump given the published file layout changes, and that Cursor's second-opinion pass produced no output.
Findings: 0 blocking | 11 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output —
cursor-review.mdis empty. This review therefore merges only my own findings with Codex's (whose single finding I believe is incorrect; see the inline note onpackage.json:9). - Changeset severity: the changeset marks all six packages
patch, but several changes alter the published file layout, not just behaviour —files: ["dist"]oncreate-seiandprecompilesdropssrc/from the tarball for the first time;ledger/registry/precompilesrename the ESM entry fromdist/esm/**/index.jstoindex.mjs; andsei-global-walletrestructuresexportsso CJS consumers now resolve types from a newdist/types-cjs/tree. Anyone deep-importing a path inside those packages breaks on upgrade. Worth consideringminorfor at least those five, or confirming deep imports are explicitly unsupported. - License hygiene is applied to five packages but
packages/mcp-server/LICENSEis left asCopyright (c) 2025 mcpdotdirectwhile every newly added LICENSE saysSei Labs. That may well be deliberate upstream attribution for a forked project, but given the PR's stated goal it's worth stating explicitly. Relatedly, the repo root still has noLICENSEfile. - No runtime smoke test for the new
sei-global-walletdual-package setup. publint and ATTW validate metadata and type resolution, but neither loads the code — a two-linerequire('@sei-js/sei-global-wallet')/import(...)check against the packed tarball would catch a regression infinalize-package.mjs(e.g.dist/cjs/package.jsonsilently not being written), which is now the single point of failure for CJS consumers. - 7 suggestion(s)/nit(s) flagged inline on specific lines.
| "scripts": { | ||
| "build:all": "pnpm -r run build", | ||
| "docs": "cd docs && pnpm dlx mint dev", | ||
| "lint:pack:all": "pnpm --recursive --filter './packages/*' exec publint run --pack npm --strict --level warning && pnpm --recursive --filter './packages/*' --filter '!@sei-js/create-sei' --filter '!@sei-js/mcp-server' exec attw --pack . --profile node16 && pnpm --filter @sei-js/mcp-server exec attw --pack . --profile esm-only", |
There was a problem hiding this comment.
[nit] Codex flagged this as a High severity blocker, claiming publint run --pack npm treats run as the directory to validate and that publint has no run subcommand. I don't think that's right — publint's CLI is sade-based and documents run [dir] as its default command alongside deps [dir] (the deps command is why the explicit run name exists in 0.3.x). run is consumed as the command, and [dir] correctly defaults to the cwd. I couldn't install the dependency in this environment to run it directly, but the PR's test plan reports pnpm lint:pack:all passing, and CI now runs it on every push, so this will be settled definitively by the first green run. Worth a glance at the CI log before merging rather than a code change.
Separate, minor point on this line: it's three chained commands with four filter expressions on one line, and the !create-sei / !mcp-server exclusions encode non-obvious reasoning (create-sei is a CLI with no type surface; mcp-server is ESM-only). When this fails in CI it'll be hard to tell which of the three invocations broke. Consider splitting into lint:pack:publint, lint:pack:types, and lint:pack:types:mcp with lint:pack:all composing them, so the failing step names itself.
| "description": "TypeScript library for Sei chain constants and assets", | ||
| "main": "./dist/cjs/src/index.js", | ||
| "module": "./dist/esm/src/index.js", | ||
| "module": "./dist/esm/src/index.mjs", |
There was a problem hiding this comment.
[suggestion] Renaming the ESM entry to .mjs is a real fix — these packages have no "type" field, so they're implicitly CommonJS, and the old dist/esm/src/index.js (emitted with --module esnext) would have thrown on its own export statements if anything loaded it with Node semantics. The .mjs extension makes it unambiguously ESM.
The gap is that ledger, registry, and precompiles still have no exports field, so Node's resolver never reaches the module field at all — module is bundler-only convention. import '@sei-js/registry' in Node still falls back to main and gets the CommonJS build; only webpack/vite/rollup will pick up the new .mjs. So the ESM output is correct now but still unreachable for the Node consumers who'd benefit most.
Adding an exports map with import/require/types conditions to these three would complete the entrypoint work this PR started. It's also what makes ATTW's node16 profile meaningful for them — right now it passes trivially because it sees a CJS-only package. Fine to defer to a follow-up, but flagging it so the current state isn't mistaken for a finished dual-package setup.
| "build": "rimraf dist && pnpm run build:cjs && pnpm run build:esm && pnpm run build:types", | ||
| "build:cjs": "tsc --outDir dist/cjs --module commonjs", | ||
| "build:esm": "tsc --outDir dist/esm --module esnext", | ||
| "build:esm": "esbuild src/index.ts --bundle --format=esm --platform=neutral --packages=external --target=es2020 --outfile=dist/esm/src/index.mjs", |
There was a problem hiding this comment.
[suggestion] Worth confirming the published size here specifically. --bundle inlines every relative import into the single output file, and registry's tsconfig pulls in ./chain-registry/**/* and ./community-assetlist/**/* — so the entire vendored registry JSON now lands inside dist/esm/src/index.mjs, whereas the CJS build (tsc) still emits it as separate sibling .json files under dist/cjs/. The two builds now have materially different shapes.
I checked the sources and there's no fs/__dirname/require() usage anywhere in registry, ledger, or precompiles, so nothing resolves those JSON files at runtime by path — the bundling is functionally safe, which is the important part. Tree-shaking should also mostly survive, since bundlers do statement-level DCE within a module and sideEffects: false is set.
The two things to sanity-check before publishing: (1) that dist/esm/src/index.mjs hasn't ballooned to something that hurts web consumers, and (2) that dropping the separate dist/esm/**/*.json files doesn't break anyone deep-importing them (which ties into the changeset-severity note). Also note esbuild doesn't type-check — that's covered here since build:cjs and build:types still run tsc over the same sources, so nothing is lost.
| @@ -0,0 +1,16 @@ | |||
| ISC License | |||
|
|
|||
| Copyright (c) 2025 Dynamic Labs, Inc. | |||
There was a problem hiding this comment.
[suggestion] This asserts that Dynamic Labs, Inc. holds copyright in this package's source. Worth confirming the basis, since the two situations call for different text:
- If
sei-global-walletis genuinely derived from Dynamic's code (the PR description says "Dynamic-derived"), then the upstream ISC notice should be reproduced verbatim as they published it, rather than paraphrased with a Sei line appended. - If it's a thin wrapper that merely depends on
@dynamic-labs/global-wallet-client— which is what the source looks like; every file here is an import/re-export of their SDK — then a dependency isn't derivation, and attributing copyright in Sei's own source to a third party is inaccurate.
Keeping "license": "ISC" to match the existing package.json field is clearly the right call either way; it's only the copyright attribution line I'd want a second look at. This is the kind of thing that's cheap to get right now and awkward to correct after it's published to npm.
| const commonJsTypesDirectory = join(packageRoot, 'dist/types-cjs'); | ||
|
|
||
| await writeFile(join(packageRoot, 'dist/cjs/package.json'), '{"type":"commonjs"}\n'); | ||
| await cp(typesDirectory, commonJsTypesDirectory, { recursive: true }); |
There was a problem hiding this comment.
[nit] This copies the whole dist/types tree to dist/types-cjs byte-for-byte, so the package now ships two identical sets of declarations purely to attach a different {"type":"commonjs"} marker. That's a legitimate and widely used technique, and I confirmed the declarations are interop-safe for it — src/index.ts uses export default Wallet, and the --module commonjs build emits exports.default with esModuleInterop, so the shared .d.ts describes both runtimes correctly.
Two small notes: it roughly doubles the type payload, and having two copies invites drift if anything ever post-processes one of them. If you want to avoid the duplication, pointing both the import and require type conditions at the single dist/types tree works when the declarations are interop-safe as they are here. Not worth blocking on — the current approach is the more conservative one and ATTW will catch it if the assumption ever stops holding.
Also consider whether this script should fail loudly: if build:cjs ever changes its output path, writeFile on line 9 will happily create dist/cjs/package.json in a directory that no longer holds the CJS build, and CJS consumers would silently get ESM files.
| run: pnpm build:all | ||
|
|
||
| - name: Validate Published Packages | ||
| run: pnpm lint:pack:all |
There was a problem hiding this comment.
[suggestion] Good placement — after build:all (both publint --pack and attw --pack need built output) and before test, so a packaging break fails fast.
One gap: this gates checks.yml, but release.yml is the workflow that actually publishes, and it doesn't run lint:pack:all. Since checks.yml triggers on all pushes including main, the Version Packages merge does get validated in practice — but as a separate job, so a red packaging check won't actually stop release.yml from publishing. Given the guidelines note that a defect here ships to every downstream dApp, adding this step to the release workflow too (or making the publish job depend on it) would close the loop.
Worth noting attw@0.18.5 requires Node >=20 and .nvmrc pins v20, so that lines up — but it leaves no headroom. If .nvmrc ever drops to 18 this step breaks rather than being skipped.
| @@ -0,0 +1,21 @@ | |||
| MIT License | |||
|
|
|||
| Copyright (c) 2026 Sei Labs | |||
There was a problem hiding this comment.
[nit] Copyright (c) 2026 on packages that have been published since 2024/2025 understates the term of copyright for the earlier releases. The conventional form is a range covering first publication to present — e.g. 2024-2026, matching the style already used in sei-global-wallet/LICENSE (2025-2026). Same applies to the identical LICENSE files added to ledger, precompiles, and registry.
Summary
zerodevand MCP declarationsLinear: PLT-847
Test plan
pnpm install --frozen-lockfile --ignore-scriptspnpm build:allpnpm lint:pack:allpnpm test:allLICENSEand no test files@sei-js/sei-global-walletpasses publint, ATTW, and tests with ISC metadata