Skip to content

fix(registry): remove install-breaking postinstall hook - #320

Merged
alexander-sei merged 1 commit into
mainfrom
fix/plt-842-registry-postinstall
Aug 10, 2026
Merged

fix(registry): remove install-breaking postinstall hook#320
alexander-sei merged 1 commit into
mainfrom
fix/plt-842-registry-postinstall

Conversation

@alexander-sei

Copy link
Copy Markdown
Contributor

Summary

  • remove the published Git submodule postinstall hook so consumers can install outside a Git checkout
  • initialize registry submodules explicitly in build and coverage CI
  • add a patch changeset and update repository guidance

Linear: PLT-842

Test plan

  • Build @sei-js/registry
  • Run all 14 registry tests
  • Pack, install, and import the package with Git discovery disabled
  • Validate changeset status and workspace diagnostics

Made with Cursor

Remove the published Git lifecycle hook so registry installs work outside repositories, while CI fetches source submodules explicitly.

Co-authored-by: Cursor <cursoragent@cursor.com>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.75%. Comparing base (2896a13) to head (28aa381).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #320   +/-   ##
=======================================
  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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@alexander-sei
alexander-sei requested a review from monty-sei August 9, 2026 18:58

@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.

Correct, well-scoped fix: the published postinstall hook ran git submodule update --init --recursive in every consumer's install directory, which fails outside a Git checkout (and mutates the consumer's own repo inside one), and the compiled dist already embeds the submodule JSON so consumers never needed it. The one gap is developer experience — nothing now initializes the submodules for a fresh local clone, and the root README's pnpm installpnpm build:all instructions will fail.

Findings: 0 blocking | 6 non-blocking | 2 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • Root README.md still says "To build all packages and docs, run pnpm install then pnpm build:all", which no longer works on a fresh clone now that nothing initializes the submodules. packages/registry/RUNBOOK.md documents git submodule update --init --recursive, but that's easy to miss from the repo root — worth adding the step (or git clone --recurse-submodules) to the README's Development section alongside this change.
  • Consider a friendlier failure mode for the registry build: if the submodules are missing, tsc fails with Cannot find module '../../chain-registry/chain_info.json', which doesn't point at the fix. A one-line guard in the build script (or a prebuild check) that prints "run git submodule update --init --recursive" would save contributors a debugging round-trip.
  • The Cursor second-opinion file (cursor-review.md) is empty — that pass produced no output, so its coverage isn't reflected here. Codex explicitly reported no material issues, matching this review.
  • Verified as correct, not a finding: the changeset (@sei-js/registry patch) matches guideline 3, resolveJsonModule + outDir means tsc emits chain_info.json and the assetlist into dist, so the packed tarball is self-contained — consistent with the "pack, install, import with Git discovery disabled" item in the test plan. No prompt-injection or instruction-like content found in the diff or PR description.
  • 2 suggestion(s)/nit(s) flagged inline on specific lines.

"build:types": "tsc --project ./tsconfig.declaration.json",
"test": "jest",
"postinstall": "git submodule update --init --recursive"
"test": "jest"

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] Removing this from the published package is right — git submodule update --init --recursive fails outside a Git checkout and, when a consumer does install inside one, it operates on their repo's submodules. But it was also the only thing initializing the submodules for local development, so a fresh clone followed by pnpm install && pnpm build:all now fails on registry with Cannot find module '../../chain-registry/chain_info.json'.

The root package.json is "private": true, so its scripts are never published. Moving the hook there keeps local dev working with zero consumer impact:

"scripts": {
  "postinstall": "git submodule update --init --recursive",
  ...
}

(Guard it with git rev-parse --git-dir > /dev/null 2>&1 && if you want it to no-op in tarball/CI contexts.) Alternatively, document the step in the root README — see the separate non-blocking note.

- uses: actions/checkout@v4
with:
fetch-depth: 0 # Fetch the full history instead of a shallow clone
submodules: recursive

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] Correct and necessary now that postinstall is gone. Minor: neither entry in .gitmodules has nested submodules, so submodules: true would do and avoids the recursive walk. Both submodule URLs are public HTTPS, so this also works for fork PRs under the default GITHUB_TOKEN — no change needed there.

@alexander-sei
alexander-sei merged commit d12ad96 into main Aug 10, 2026
13 checks passed
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