fix: standardize package publishing hygiene (PLT-847) - #321
fix: standardize package publishing hygiene (PLT-847)#321alexander-sei wants to merge 4 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 82.59% 82.59%
=======================================
Files 79 79
Lines 1333 1333
Branches 163 224 +61
=======================================
Hits 1101 1101
+ Misses 232 226 -6
- Partials 0 6 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
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.
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.
Merge the latest main branch and tighten release validation, package licensing, and MCP project configuration. Co-authored-by: Cursor <cursoragent@cursor.com>
PR SummaryMedium Risk Overview Publishing metadata and contents: Adds Build and types: Several libraries switch ESM output to esbuild Automation: Root adds Reviewed by Cursor Bugbot for commit 2fb5145. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Solid, well-scoped packaging-hygiene PR: LICENSE files, files fields, dual ESM/CJS type resolution, test artifacts excluded from builds, and publint/ATTW gates in CI — all backed by a six-package changeset. No correctness or security regressions found (the mcp-server Node16 switch and the create-sei/sei-global-wallet files narrowing both verify clean); findings are a self-cancelling tsconfig, a release-gating dependency-pinning risk, and a few hygiene notes.
Findings: 0 blocking | 11 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty); Codex reported no material issues. Only this pass and Codex effectively covered the diff. packages/mcp-server/LICENSEstill readsCopyright (c) 2025 mcpdotdirectwhile all five new LICENSE files attribute Sei Labs. If mcp-server is derived from mcpdotdirect/evm-mcp-server this is correct and must be retained — but please confirm it was a deliberate skip rather than an oversight in a licensing-hygiene PR, and consider adding a Sei Labs copyright line alongside the upstream one.- The root
package.jsondeclares"license": "MIT"but there is no LICENSE file at the repo root. Cheap to add while you're in here. - The test plan verifies "all six packed packages contain LICENSE and no test files" manually; nothing in CI asserts it. publint/ATTW check entrypoint and type resolution, not tarball contents. A small script over
npm pack --jsonoutput (assertLICENSEpresent, no*.test.*/tests/entries) would keep the invariant from silently regressing. lint:pack:typesexcludes@sei-js/create-seifrom ATTW entirely. That is correct — it's a bin-only package with no types entry — but the reason isn't recorded anywhere. A one-line comment or a note in the changeset would keep a future reader from "fixing" the filter.@sei-js/ledger,@sei-js/registryand@sei-js/precompilesnow ship a single bundled.mjsas themoduleentry, replacing the per-moduledist/esm/**/*.jstree. Since none of these packages declare anexportsfield, deep imports underdist/esm/were reachable and now 404. Low risk, but worth a sentence in the changeset. (For@sei-js/registrythis is a genuine improvement: the previous tsc ESM output emitted bare relative imports of the submodule JSON, whereas esbuild inlines it.)- 5 suggestion(s)/nit(s) flagged inline on specific lines.
| "resolveJsonModule": true | ||
| "resolveJsonModule": true, | ||
| "module": "ES2020", | ||
| "moduleResolution": "Bundler" |
There was a problem hiding this comment.
[suggestion] This project now resolves to zero input files. tsconfig.json gained "exclude": [..., "src/tests/**/*", "**/*.test.ts", ...], and exclude is inherited through extends unless the child overrides it. TypeScript applies exclude as a filter over include, so "include": ["src/tests/**/*.test.ts"] on line 10 is fully cancelled out.
Nothing breaks today because ts-jest reads only compilerOptions from this file and gets its file list from Jest — but the config is self-contradicting: tsc -p tsconfig.test.json would fail with TS18003, and tsserver will no longer associate test files with this project, so describe/expect lose the types: ["jest", "node"] setting in-editor.
Add an override alongside the new compiler options:
"exclude": ["node_modules", "dist"],| "@changesets/cli": "^2.28.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@arethetypeswrong/cli": "^0.18.5", |
There was a problem hiding this comment.
[suggestion] lint:pack:all is now a gate on release.yml (before the changesets publish step), so these two tools sit on the publish critical path. With caret ranges, a minor release of publint or ATTW that adds a new rule — or tightens an existing one under --strict --level warning — will fail the release job on an unrelated commit and block publishing until someone intervenes.
Consider pinning both exactly ("@arethetypeswrong/cli": "0.18.5", "publint": "0.3.23") and bumping them deliberately, so rule changes land in a reviewable PR rather than mid-release.
Keep the MCP test project active and make publish validation deterministic and easier to diagnose. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Solid, well-scoped packaging-hygiene PR: LICENSE files, files/publishConfig, correct dual ESM/CJS conditional exports for sei-global-wallet, test artifacts excluded from builds, and publint/ATTW wired into both CI and release — plus a changeset covering all six packages, as the repo guidelines require. No blockers found; the notes below are about the esbuild-based ESM rewrite going unvalidated by the very checks this PR adds, a pre-existing postinstall that ships to consumers, and a couple of redundancies.
Findings: 0 blocking | 10 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Coverage gap in the new gate:
lint:pack:typesruns ATTW with--profile node16, which only exercisesexports/main/types. Since ledger/precompiles/registry have noexportsmap, the new esbuild.mjsbundles — the riskiest change in this PR — are reachable only via the bundler-onlymodulefield and are therefore validated by nothing in CI. A one-line smoke check (e.g.node --input-type=module -e "import('./dist/esm/src/index.mjs')") per package, or anexportsmap, would close it. - Cursor's second-opinion pass produced no output (
cursor-review.mdis empty). Codex's pass ran and reported no material issues. Only two of the three reviews contributed findings here. @sei-js/sei-global-walletstays ISC while the other five packages and the repo root are MIT. The PR deliberately preserves this and updates the README badge to match, which is the right call for a metadata-hygiene PR — worth a one-line confirmation from a maintainer that ISC is intentional rather than historical drift, since this is the PR that makes it durable by adding a LICENSE file.- Verified clean, for the record: every relative import in
sei-global-wallet/srcnow carries.jswith no misses (ethereum.ts/zerodev.tshave only bare-specifier re-exports, so they correctly needed no change); all 112 relative imports inmcp-server/srcalready carried.js, so the switch tomodule/moduleResolution: Node16is safe, and both jest configs already map^(\.{1,2}/.*)\.js$back to source;package-info.tsstill resolves../../../package.jsonbecause thedist/esmlayout is unchanged and npm always packspackage.jsonregardless offiles;mcp-serveralready had a LICENSE, so the five new ones cover the actual gap; none of the esbuild-bundled sources userequire/__dirname/process.env, so the neutral-platform ESM output is safe; and the security-criticaltransport/security.tsandtransport/http-sse.tsare untouched. No prompt-injection attempts in the diff or PR body. - 6 suggestion(s)/nit(s) flagged inline on specific lines.
| "build:esm": "esbuild src/index.ts --bundle --format=esm --platform=neutral --packages=external --target=es2020 --outfile=dist/esm/src/index.mjs", | ||
| "build:types": "tsc --project ./tsconfig.declaration.json", | ||
| "test": "jest", | ||
| "postinstall": "git submodule update --init --recursive" |
There was a problem hiding this comment.
[suggestion] Pre-existing, but squarely in this PR's scope: postinstall ships inside the published tarball, so npm i @sei-js/registry runs git submodule update --init --recursive in the consumer's project. Outside a git repo it errors; inside one it operates on their repo's submodules, not ours. Since this PR is the "publishing hygiene" pass, consider moving submodule bootstrapping to a root-level prepare/dev script (or guarding on !process.env.CI && existsSync('.git')) so it never reaches consumers.
| "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] Two notes on the esbuild switch, both of which also apply to precompiles and registry:
- The bundle is only reachable through the non-standard
modulefield. With noexportsmap, Node ESMimport '@sei-js/ledger'still resolvesmain→dist/cjs/src/index.js(CJS), so the ESM/CJS entrypoint fix stated in the PR description lands only for bundlers. Adding anexportsmap would make it real and bring the.mjsunder ATTW'snode16profile. - Renaming the output
.js→.mjsopts webpack intojavascript/esmmodule type instead ofjavascript/auto, which enforces strict ESM interop. Named imports from CJS-only deps that previously worked can start failing — for ledger specifically,import { SeiApp } from '@zondax/ledger-sei'. Worth confirming against a real bundler consumer before release;import Transport from '@ledgerhq/hw-transport-node-hid'(default import) is fine either way.
| "@arethetypeswrong/cli": "0.18.5", | ||
| "@types/jest": "^29.5.14", | ||
| "@types/node": "^22.13.13", | ||
| "esbuild": "^0.28.2", |
There was a problem hiding this comment.
[suggestion] publint and @arethetypeswrong/cli are exact-pinned, but esbuild is a caret range — and unlike the other two it now produces the published artifacts for three packages. The lockfile covers CI, but pinning it exactly matches the intent of the other two pins and keeps published output reproducible.
Separately: esbuild is only a root devDependency while packages/{ledger,precompiles,registry} invoke it from their own build scripts. That resolves today via pnpm putting the workspace-root .bin on PATH, but breaks if any of those packages is ever built standalone. Declaring it as a devDependency of the three packages that use it would be more robust.
| @@ -0,0 +1,4 @@ | |||
| { | |||
| "extends": "./tsconfig.json", | |||
| "exclude": ["node_modules", "dist", "src/tests/**/*", "**/*.spec.ts", "**/*.test.ts", "**/*.spec.js", "**/*.test.js", "**/__tests__/**/*"] | |||
There was a problem hiding this comment.
[nit] This exclude is byte-identical to the one added to tsconfig.json in the same PR, so tsconfig.build.json currently inherits everything and overrides nothing. Either drop this file and point tsconfig.declaration.json back at tsconfig.json, or drop the exclude from tsconfig.json and let this file be the thing that carries it. As-is the two lists will silently drift.
| 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] A short header comment would help here — the dist/types-cjs duplication is non-obvious (it exists so the require condition resolves declarations under a {"type":"commonjs"} scope, which is what makes ATTW pass now that the root package is "type": "module"). Worth noting the trade-off too: it roughly doubles the published .d.ts payload.
Also note this only runs from the full build script, so pnpm build:cjs on its own leaves dist/cjs without its package.json marker and reintroduces the dual-package hazard. Harmless today since build starts with rimraf dist, but a comment saying "must run after all three builds" would prevent someone reordering the chain later.
| "license": "MIT", | ||
| "scripts": { | ||
| "build:all": "pnpm -r run build", | ||
| "lint:pack:all": "pnpm lint:pack:publint && pnpm lint:pack:types && pnpm lint:pack:types:mcp", |
There was a problem hiding this comment.
[nit] Chaining with && means a publint failure short-circuits before ATTW ever runs, so a CI run surfaces one class of packaging problem at a time. Minor, but for a validation aggregate it's usually nicer to run all three and fail at the end.
2 participants
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