fix(cli): ship scripts/build/runtime-env.mjs in npm package (#5227) - #5230
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds the missing 'scripts/build/runtime-env.mjs' script to the package.json 'files' whitelist and introduces a regression test to ensure all scripts imported by CLI entrypoints are packaged. The reviewer suggested improving the test's regex scanner by stripping comments from the source files first to avoid false positives from commented-out imports.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const src = readFileSync(binFile, "utf8"); | ||
| const specifiers = new Set<string>(); | ||
| // static: from "..." dynamic: import("...") | ||
| const re = /(?:from|import)\s*\(?\s*["']([^"']+)["']/g; |
There was a problem hiding this comment.
The current regex-based import scanner can match commented-out imports (e.g., // import ...), which may lead to false-positive test failures if a developer comments out an import during debugging or refactoring. Stripping single-line and multi-line comments from the source code before running the regex will make the test more robust.
| const src = readFileSync(binFile, "utf8"); | |
| const specifiers = new Set<string>(); | |
| // static: from "..." dynamic: import("...") | |
| const re = /(?:from|import)\s*\(?\s*["']([^"']+)["']/g; | |
| const rawSrc = readFileSync(binFile, "utf8"); | |
| const src = rawSrc.replace(/\/\*[\s\S]*?\*\/|\/\/.*$/gm, ''); | |
| const specifiers = new Set<string>(); | |
| // static: from "..." dynamic: import("...") | |
| const re = /(?:from|import)\s*\(?\s*["']([^"']+)["']/g; |
15b4ed4 to
e631672
Compare
serve.mjs imports scripts/build/runtime-env.mjs (added with the #5213 heap auto-calibration fix) but it was missing from package.json files whitelist, breaking every global npm install at startup. Add it to files and guard with a regression test that asserts all bin/ runtime imports of scripts/ are packaged.
e631672 to
fbd1f91
Compare
…zapw#5227) (diegosouzapw#5230) serve.mjs imports scripts/build/runtime-env.mjs (added with the diegosouzapw#5213 heap auto-calibration fix) but it was missing from package.json files whitelist, breaking every global npm install at startup. Add it to files and guard with a regression test that asserts all bin/ runtime imports of scripts/ are packaged.
1 participant
Closes #5227
Problem
The v3.8.39 heap auto-calibration fix (#5213) made
bin/cli/commands/serve.mjsimportscripts/build/runtime-env.mjs, but that file was never added to thefileswhitelist inpackage.json. The published npm tarball therefore shipped the importing CLI without the imported module, so everynpm install -g omniroutefailed at startup:Confirmed on macOS (#5227, @PriyomSaha), and reproduced by @m-Yaghoubi and @jonlwheat2-gif (Windows 10).
Fix
scripts/build/runtime-env.mjsto thefileswhitelist inpackage.json(the other twobin/→scripts/runtime imports —native-binary-compat.mjs,postinstallSupport.mjs— were already whitelisted; only the new one was missing).npm pack --dry-runnow confirms the file ships.Regression test (TDD)
tests/unit/cli-runtime-imports-packaged-5227.test.tsscans everybin/**entrypoint for static/dynamic imports resolving underscripts/and asserts each resolved path is covered by the packagefileswhitelist. Fails on the unfixed tree (runtime-env.mjsuncovered) and passes after the whitelist entry — and guards against any future unpackagedbin/→scripts/import failing users' installs instead of CI.