Repository navigation
fix(ci): reuse npm downloads across lockfile updates - #1659
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🔵 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.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Verified the pinned-action compatibility noted in Copilot's review:
No code change was needed for this compatibility question. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Final-head confirmation for Copilot: node-ci passed on |
|
Post-merge verification passed on main commit |
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-offlinecannot reuse downloads that were never restored.Changes
node_modules, omit dependencies, or change timeouts.%sfor numbers and booleans, so Bun emits distinct JUnit identities. Keep strict inventory comparison and all test cases enabled.Testing
bun test --timeout 30000 sdk/typescript/tests-ts/skeleton.test.ts: 18 passed.pnpm run typesandpnpm 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.12345had 4,898 passed / 68 skipped / 11 failed; seed3526816371had 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