Skip to content

fix(ci): reuse npm downloads across lockfile updates - #1659

Merged
mldangelo-oai merged 3 commits into
mainfrom
mdangelo/codex/windows-npm-install-performance
Oct 16, 2026
Merged

mldangelo-oai merged 3 commits into
mainfrom
mdangelo/codex/windows-npm-install-performance

Conversation

@mldangelo-oai

@mldangelo-oai mldangelo-oai commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Avoid repeated cold npm installs when the SDK lockfile changes. The pinned Node setup action restores only the exact lockfile-keyed npm cache, so an unrelated dependency update discards cached downloads for unchanged packages, including the roughly 165 MB Windows Codex archive. --prefer-offline cannot reuse downloads that were never restored.

Changes

  • On an exact cache miss, restore the most recent npm download cache for the same operating system and architecture.
  • Keep the existing lockfile-specific primary key and Node setup action's cache saving behavior.
  • Continue installing and verifying the complete package on every run; do not cache node_modules, omit dependencies, or change timeouts.
  • Test the fallback wiring and execute the workflow's cache-key resolver with different lockfile hashes, platforms, architectures, and paths containing spaces.
  • Fix three existing parameterized test titles exposed by the workflow's full runner-quality check: use numeric/JSON placeholders instead of %s for numbers and booleans, so Bun emits distinct JUnit identities. Keep strict inventory comparison and all test cases enabled.

Testing

  • Windows cold-cache versus fallback-cache profiling run: both complete installed-package checks passed. npm install took 161.4 seconds cold versus 93.7 seconds with fallback; fallback avoided 113 npm registry tarball downloads. Restoring the GitHub cache took 8 seconds.
  • The profiler deliberately used an unmatched primary key and verified that it restored an existing cache. Its temporary workflow is removed from the final diff.
  • bun test --timeout 30000 sdk/typescript/tests-ts/skeleton.test.ts: 18 passed.
  • SDK pnpm run types and pnpm run format: passed.
  • actionlint -shellcheck= .github/workflows/node-ci.yml: passed; shellcheck is covered by CI rather than this local invocation.
  • git diff --check: passed.
  • The first PR run passed every test/build job, but its report-comparison step found three duplicate test identities. The test-title correction addresses that validation blocker; the final head must pass the complete CI matrix before merge.
  • Six affected parameterized cases pass with unique JUnit identities, and strict report self-comparison passes. The report-comparison and skeleton suites pass together: 29 tests.
  • Full local SDK runs: seed 12345 had 4,898 passed / 68 skipped / 11 failed; seed 3526816371 had 4,908 passed / 68 skipped / 1 failed. The failures were execution-environment issues (an ancestor Git marker and child React dependency resolution), not changed code. All affected cases passed after correcting the working directory and dependency resolution; hosted CI's actual test jobs all passed.

Risk and rollout

CI-only change; no public CLI, package dependency, authentication, or runtime changes. npm still validates and installs the requested package versions from its content-addressed download cache. Platforms and architectures remain isolated.

Unpacking remains the dominant npm phase, and hosted-runner variance means the timing comparison is not a guaranteed speedup. Neither profiling install reproduced the original 300-second timeout; this fixes the demonstrated cache invalidation problem rather than claiming to eliminate every possible install stall. Merge only after Codex and Copilot review and passing CI, then verify CI on the merged main commit.

Public disclosure review

  • No customer, partner, prospect, or user identities, data, or identifying details are included.
  • No credentials, personal data, private source, scan findings, or nonpublic links or tickets are included.
  • I reviewed the branch name, title, description, commits, changes, comments, logs, screenshots, attachments, and links for public disclosure.

@github-actions github-actions Bot added the bug Something isn't working label Oct 16, 2026
@mldangelo-oai

Copy link
Copy Markdown
Collaborator Author

@codex review

@mldangelo-oai
mldangelo-oai requested a balanced review from Copilot October 16, 2026 01:55
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-16T03:10:40.736785Z 9c3abd7 Manual request
🔒 Security Review ✅ Completed 2026-10-16T03:12:45.480956Z 9c3abd7 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

Copilot AI 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.

🔵 Needs a closer look

The pinned setup-node action’s cache-key output contract and compatibility with the fallback restore action remain unverified.

0 open findings

What changed in this PR

This PR adds npm download-cache fallback for SDK CI when lockfile changes prevent an exact cache hit, while preserving full package installation and verification.

Changes:

  • Restore previous downloads for the same operating system and architecture.
  • Preserve lockfile-specific cache keys and existing save behavior.
  • Test fallback wiring and key resolution across platforms, architectures, and paths containing spaces.
File Description
sdk/​typescript/​tests-ts/​skeleton.test.ts Tests cache fallback wiring and resolver behavior.
.github/​workflows/​node-ci.yml Adds fallback restoration to compatibility and Windows verification jobs.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Can't wait for the next one!

Reviewed commit: ae9f7594e6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@mldangelo-oai

Copy link
Copy Markdown
Collaborator Author

Verified the pinned-action compatibility noted in Copilot's review:

  • The pinned setup-node implementation constructs node-cache-<OS>-<arch>-npm-<lockfile hash> and emits cache-primary-key before attempting restoration: source.
  • The fallback uses the same npm config get cache directory and keeps the OS/architecture prefix. In the Windows experiment, the pinned cache/restore action successfully restored an existing setup-node cache using a deliberately unmatched primary key. The full installed-package verification passed, with no npm registry tarball GETs.
  • setup-node remains the only cache saver. Its post step saves the current primary key when its own initial restore missed: source.

No code change was needed for this compatibility question.

@mldangelo-oai

Copy link
Copy Markdown
Collaborator Author

@codex review

Copilot AI 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.

🔵 Needs a closer look

The pinned setup-node action’s cache-key output contract and final-head CI results still need confirmation.

0 open findings

🧠 Review effort: Balanced

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Keep it up!

Reviewed commit: 9c3abd777e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@mldangelo-oai

Copy link
Copy Markdown
Collaborator Author

Final-head confirmation for Copilot: node-ci passed on 9c3abd777eea98ab3f4249112ddb562ce444e47f, including both Windows installed-package jobs and the strict runner inventory comparison. The pinned setup-node output contract and real cross-key restore were verified in the earlier evidence response; that cache implementation is unchanged. Codex code/security and Copilot reviews report zero findings, with no unresolved review threads.

@mldangelo-oai
mldangelo-oai merged commit 5486000 into main Oct 16, 2026
86 checks passed
@mldangelo-oai
mldangelo-oai deleted the mdangelo/codex/windows-npm-install-performance branch October 16, 2026 04:17
@mldangelo-oai

Copy link
Copy Markdown
Collaborator Author

Post-merge verification passed on main commit 54860007cc2a8b3dc30083017359556161c459b6: node-ci completed with 49 successful jobs and 1 skipped, including Windows Node 22 and 24 installed-package verification. The main CodeQL, code-quality, and release-PR workflows also passed. No follow-up fix is indicated by these results.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants