Guard node_modules cache against missing native optional-dep binaries#324064
Draft
Giuspepe wants to merge 2 commits into
Draft
Guard node_modules cache against missing native optional-dep binaries#324064Giuspepe wants to merge 2 commits into
Giuspepe wants to merge 2 commits into
Conversation
Some deps ship their native binary as a per-platform *optional* dependency of a small launcher package (e.g. `@openai/codex` -> `@openai/codex-<platform>-<arch>`). npm does not fail when an optional dependency can't be installed, so a transient hiccup can leave the launcher present but the binary missing — and that broken tree then gets frozen into the shared node_modules cache and served to every consumer (see #323881). Add build/azure-pipelines/common/checkNativeOptionalDeps.ts and run it after `npm ci` in the linux, macOS and windows cache-build jobs (before the cache is saved). It fails the job when a launcher package is present but its platform binary is missing, so a poisoned cache is never saved. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Hardens the node_modules cache build workflow by adding a post-npm ci guard that detects when a launcher package is present but its platform-specific native optional-dependency binary is missing, preventing poisoned caches from being saved and reused across jobs.
Changes:
- Added a data-driven native optional-dependency verifier script (
checkNativeOptionalDeps.ts) that checks for expected binaries undervendor/*/bin/when the launcher package is installed. - Integrated the verification step into Linux, macOS, and Windows
pr-node-modulescache-build jobs on cache misses, before saving the cache.
Show a summary per file
| File | Description |
|---|---|
| build/azure-pipelines/common/checkNativeOptionalDeps.ts | Adds a post-install check that fails the cache build if required native optional-dependency binaries are missing. |
| .github/workflows/pr-node-modules.yml | Runs the new verifier after npm ci (on cache misses) and before saving node_modules caches on Linux/macOS/Windows. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Low
Giuspepe
force-pushed
the
harden-node-modules-cache
branch
from
16ff12e to
40dc39c
Compare
DO NOT MERGE — dropped before this PR is ready. Adds a branch trigger and bumps build/.cachesalt to force a cache miss so npm ci + the verify step actually run; no binary is removed, so the verify step should pass on all platforms. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Follow-up hardening for #323881. Some dependencies ship their native binary as a per-platform optional dependency of a small launcher package — e.g.
@openai/codexis only a launcher shim, and the real binary lives in@openai/codex-<platform>-<arch>.npm install/npm cido not fail when an optional dependency can't be installed, so a transient hiccup can leave the launcher present while the platform binary is missing. That broken tree then gets frozen into the sharednode_modulescache and served to every consumer (this is exactly what poisoned the Linux cache and broke the Codex smoke test).There is no npm flag to make an optional-dependency install failure fatal (and npm can't tell a legitimate platform-skip from a failed fetch), so the only reliable guard is an explicit post-install check.
What
build/azure-pipelines/common/checkNativeOptionalDeps.ts: afternpm ci, verify the current platform's expected native binary exists. If the launcher package is present but its per-platform binary is missing, exit non-zero with an actionable message. If the launcher isn't installed at all, it's a no-op.pr-node-modules.yml, after the rootnpm ciand before the cache is saved (gated on a cache miss, i.e. only when an install actually ran).Result: a
node_modulescache missing a required native binary can never be saved — the job fails loudly at build time instead of silently poisoning every downstream consumer.The check is data-driven (a small list of native optional deps), so adding future ones (e.g. other bundled native SDKs) is a one-line change.
Notes
@openai/codex.compile/copilot cache jobs are out of scope (their caches aren't consumed where the codex binary is required).