fix(docker): prune rule deletes runtime modules named like docs - #836
Conversation
… docs The bare `-iname 'CHANGES*'` glob (and siblings) in the `PRUNE_MODULES` pass at Dockerfile:84-99 deletes any file whose case-insensitive name starts with the token. That includes runtime JS modules: `@mysten/sui/dist/cjs/client/types/changes.js` is shipped by Sui SDK and required by `client/types/index.js`. After prune the file is gone, so `Cannot find module './changes.js' from .../client/types/index.js` crashes the node on boot when any transitive code path hits @mysten/sui (via rubic-sdk → cetus-sui-clmm-sdk → demos SDK). Replace each `-iname 'X*'` with an explicit set of extensionless + doc-extension forms (`X`, `X.md`, `X.txt`, `X.rst`). README.md / CHANGELOG.md / LICENSE.txt etc still pruned; `changes.js` and any other `<doc-token>.js` runtime file stays. Verified by rebuild + `ls node_modules/@mysten/sui/dist/cjs/client/types/changes.js` present in the resulting image. No code changes elsewhere. Bare-metal `./run` flow unchanged (prune is docker-only).
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
Greptile SummaryThis PR fixes a Docker boot crash caused by the
Confidence Score: 4/5The change is safe to merge; it eliminates an active boot crash with no risk of introducing new module-deletion side effects. The fix is well-scoped and directly addresses the confirmed crash. The only gap is the missing plural NOTICES variants from the original list, which is a minor pruning regression and does not affect correctness or startup reliability. Dockerfile lines 84–110 (the PRUNE_MODULES find expression) — specifically the NOTICES omission and the completeness of the license variant list if new dependencies are added later. Important Files Changed
Reviews (1): Last reviewed commit: "fix(docker): prune rule deletes legitima..." | Re-trigger Greptile |
| -iname 'CONTRIBUTORS' -o -iname 'CONTRIBUTORS.md' -o -iname 'CONTRIBUTORS.txt' -o \ | ||
| -iname 'LICENSE' -o -iname 'LICENSE.md' -o -iname 'LICENSE.txt' -o -iname 'LICENSE-MIT' -o -iname 'LICENSE-APACHE' -o -iname 'LICENSE-MIT.txt' -o -iname 'LICENSE-APACHE.txt' -o \ | ||
| -iname 'LICENCE' -o -iname 'LICENCE.md' -o -iname 'LICENCE.txt' -o \ | ||
| -iname 'NOTICE' -o -iname 'NOTICE.md' -o -iname 'NOTICE.txt' -o \ |
There was a problem hiding this comment.
The original list had a separate
-iname 'NOTICES*' entry (distinct from NOTICE*). The new list covers NOTICE, NOTICE.md, NOTICE.txt but not the plural NOTICES, NOTICES.md, NOTICES.txt — which some packages (e.g. Apache-licensed Java/Node libraries) do ship. This is a minor pruning regression rather than a correctness bug, but worth carrying over for completeness.
| -iname 'NOTICE' -o -iname 'NOTICE.md' -o -iname 'NOTICE.txt' -o \ | |
| -iname 'NOTICE' -o -iname 'NOTICE.md' -o -iname 'NOTICE.txt' -o \ | |
| -iname 'NOTICES' -o -iname 'NOTICES.md' -o -iname 'NOTICES.txt' -o \ |
| -iname 'governance.md' -o \ | ||
| -iname 'security.md' -o \ |
There was a problem hiding this comment.
governance.md and security.md use lowercase patterns but sit alongside entries that are conventionally uppercased. Since all entries use -iname (case-insensitive), this is harmless at runtime — it is purely a consistency observation.
| -iname 'governance.md' -o \ | |
| -iname 'security.md' -o \ | |
| -iname 'GOVERNANCE.md' -o \ | |
| -iname 'SECURITY.md' -o \ |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Disabled knowledge base sources:
WalkthroughThe Dockerfile's builder stage ChangesDocker build stage pruning logic
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Problem
Containerised node crashes on boot with:
Root cause
Dockerfile:84-99runs an aggressivePRUNE_MODULESpass to strip docs and license files fromnode_modules. The rule uses case-insensitive prefix globs:CHANGES*matcheschanges.js(case-insensitive) — Sui's actual runtime module@mysten/sui/dist/cjs/client/types/changes.js. After prune, the file is missing andclient/types/index.jsfails itsrequire('./changes.js').Same overreach exists for every
-iname 'X*'entry in the list (README*,HISTORY*,CONTRIBUTING*,AUTHORS*,LICEN[CS]E*,NOTICE*,PATENTS*,code_of_conduct*).Triggered now because the snapshot-restore branch loads
@kynesyslabs/demosdkpaths that transitively pull in @mysten/sui; older code paths happened to avoidclient/types/index.js.Fix
Replace each
-iname 'X*'with an explicit set of extensionless + doc-extension forms (X,X.md,X.txt,X.rst, plus the commonLICENSE-MIT.txt/LICENSE-APACHE.txtvariants). Real docs still pruned (~60-100 MB savings preserved); runtime JS modules with doc-sounding names stay.Verification
After rebuild:
$ docker run --rm demos-node:local ls /app/node_modules/@mysten/sui/dist/cjs/client/types/changes.js /app/node_modules/@mysten/sui/dist/cjs/client/types/changes.js $ docker run --rm demos-node:local ls /app/node_modules/@mysten/sui/README.md ls: cannot access '/app/node_modules/@mysten/sui/README.md': No such file or directoryBoth required behaviors preserved.
Test plan
docker compose build nodesucceeds.docker run --rm demos-node:local ls /app/node_modules/@mysten/sui/dist/cjs/client/types/changes.jssucceeds../run --docker --clean -dboots the node without theCannot find module './changes.js'error.Summary by CodeRabbit