Repository navigation
Conversation
…r-js deps trusted-server-core depends on trusted-server-js, whose build script runs the TSJS build with the npm on PATH. Most Rust CI jobs never set up the .tool-versions Node, so they built the embedded bundles with the runner image Node (22 on ubuntu-24.04). Add setup-node with node-version-file and npm caching to test-cloudflare, test-spin, test-parity and format-rust, and to the shared integration-test composite action before its Rust builds. Drop the now-redundant setup-node step from the integration-tests job. Remove the unused direct trusted-server-js dependency from the Cloudflare and Spin adapters; both still get it through trusted-server-core. Closes #1204 Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review summary
Reviewed 2b1914286d53d01a768850c237b7274865b24037 against 7a0ecb4cbfa39af3d83f7e1ed2733d7eabdc4cb1. All seven changed files were inspected, including CI callers and the adapter-to-core-to-JS dependency and serving paths. No actionable issues introduced by this PR were found.
Safety proof
- Logs from all eight affected CI jobs confirmed Node
v24.12.0before their first TSJS build. Wrangler installation and the browser job's separate two-lockfile cache also succeeded. - Locked, offline dependency checks confirmed both adapters retain JS through core on native and production WASM targets. Full manifest and lockfile comparison found only the two intended dependency-edge removals.
Validation and review context
- Local
cargo fmt --all -- --check, diff whitespace checks, actionlint with ShellCheck, locked/offline Cargo metadata and dependency-tree checks, and in-memory manifest, lockfile, consumer, and CI-log assertions passed. - All reported PR checks passed. Inspected logs confirmed 1,185 JS tests, native adapter tests, production-target compilation, parity, integration, and browser tests passed. CI merge commit
0002055156cf3acb3eaa604fc045ced11f0adb7chas the same tree as the reviewed head. - Existing reviews, inline comments, issue comments, and review threads contained no feedback.
- Manual deployment was not exercised. Build and runtime evidence was reused from verified same-tree CI rather than rerun locally. Review performed without delegation or file edits; worktree remained clean.
aram356
left a comment
There was a problem hiding this comment.
Summary
This PR sets up the .tool-versions Node before every Rust build in CI and drops the unused trusted-server-js dependency from the Cloudflare and Spin adapters. The core change holds up. A step-order check that expands the composite action finds 7 jobs building core before a pinned Node at the merge-base and 0 at this head, and 0 on a trial merge with current main. Every touched job's log shows node: v24.12.0, and cargo metadata --locked passes. The four inline comments below keep the restored rule from drifting again and track the remaining local-build gap. Please address them before merge.
1 of the inline comments below carries a one-click GitHub
suggestion; use Commit suggestion to apply it. The other three describe the change in prose because it touches lines outside the diff, adds a new check, or needs a follow-up issue.
Blocking
🔧 wrench
- Composite action comment makes the Node step look optional: see inline at
.github/actions/setup-integration-test-env/action.yml:48 - Older Node steps are named for a narrower purpose: see inline at
.github/workflows/test.yml:145 - Commit the step-order check: see inline at
.github/workflows/format.yml:62 - File an issue for unpinned local builds: see inline at
.github/workflows/test.yml:180
CI Status
- cargo test: PASS (required)
- cargo fmt: PASS (required)
- format-typescript: PASS (required)
- format-docs: PASS (required)
- CLAUDE.md symlink guard: PASS
- cargo test (axum native): PASS
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- cargo test (ts CLI, native) (ubuntu-latest): PASS
- cargo test (ts CLI, native) (macos-latest): PASS
- vitest: PASS
- prepare integration artifacts: PASS
- integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- browser integration tests: PASS
- Analyze (actions): PASS
- Analyze (rust): PASS
- Analyze (javascript-typescript) (CodeQL Advanced): PASS
- Analyze (javascript-typescript) (CodeQL - Code Quality): PASS
- Analyze (python): PASS
- CodeQL: PASS
Add scripts/check-ci-node-pin.py and a CI Node pin guard job that fail when a job compiles Rust before an unconditional actions/setup-node step pinned to .tool-versions, inlining local composite actions. Name and comment the existing Node steps in test-rust, test-axum and test-cli for the TSJS build, and note in the integration setup action that its Node step must stay unconditional. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Summary
trusted-server-coredepends ontrusted-server-js, whose build script runsnpm ci/npm run buildwith thenpmon PATH and embeds the bundles. Most Rust CI jobs never set up the.tool-versionsNode, so they built those bundles with the runner image's Node (22 onubuntu-24.04) instead of the pinned 24.12.0. They would also have missed the planned pin bump (Upgrade to Rust 1.98.1 and align dependencies with EdgeZero #1123).CI Node pin guardjob that fails when any job compiles Rust before an unconditionalsetup-nodestep pinned to.tool-versions, so the ordering can't regress.trusted-server-jsdependency from the Cloudflare and Spin adapters, as Remove unused dependencies and disable unnecessary test targets #614 did for Fastly. Both still get it through core.Changes
.github/workflows/test.ymlsetup-node(node-version-file: .tool-versions,cache: npm) before cargo intest-cloudflare,test-spin,test-parity; name and comment the existing Node steps intest-rust,test-axum,test-clifor the TSJS build.github/workflows/format.ymlcargo fmt/ clippy informat-rust; newcheck-ci-node-pinjobscripts/check-ci-node-pin.pyworker-build, directly or through repo.shscriptsscripts/README.md.github/actions/setup-integration-test-env/action.ymlprepare-artifacts,integration-tests-fastly-ec,browser-tests; a comment notes the step must stay unconditional.github/workflows/integration-tests.ymlintegration-testsjob'ssetup-nodestep, now done by the composite actioncrates/trusted-server-adapter-cloudflare/Cargo.tomltrusted-server-jsdependencycrates/trusted-server-adapter-spin/Cargo.tomltrusted-server-jsdependencyCargo.lockNotes:
mainalready set up Node before cargo intest-axumandtest-cli, so those jobs are unchanged.browser-testskeeps its ownsetup-nodestep for its two-lockfile npm cache.integration-tests-fastly-ec/browser-tests, and enforcing the Node version locally (enginesfield or abuild.rscheck), tracked in Enforce the pinned Node version for local and deploy builds of trusted-server-js #1266.Closes
Closes #1204
Test plan
cargo test-fastly && cargo test-axumcargo clippy-fastly && cargo clippy-axumcargo fmt --all -- --checkcd crates/trusted-server-js/lib && npx vitest runcd crates/trusted-server-js/lib && npm run formatcd docs && npm run formatcargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1fastly compute servecargo check-cloudflare,cargo check-spin,cargo test-cloudflare,cargo test-spin, and clippy for Cloudflare (native + wasm) and Spin (native + wasm).python3 scripts/check-ci-node-pin.pyreports 0 violations at this head (8 jobs on the tree before this PR) and catches deliberately broken workflow copies.Vitest passes 1185/1185 under the pinned Node 24.12.0. Under Node 26, 27 Permutive/Sourcepoint tests fail because Node 26's built-in
localStoragehides jsdom's. That is local version drift, not caused by this PR. To confirm the CI change, check that each touched job's setup step shows the.tool-versionsNode.Checklist
unwrap()in production code — useexpect("should ...")(no Rust code changed)logmacros (notprintln!) (no Rust code changed)