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>
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
This PR pins the .tool-versions Node before every Rust build in CI, adds a step-order guard so that stays true, and drops the unused trusted-server-js dependency from the Cloudflare and Spin adapters. The core change is sound and the earlier blocking comments are resolved. The remaining comments are non-blocking ways to make the guard catch more.
What I checked:
- Guard results:
check-ci-node-pin.pyflags exactly 8 jobs on the merge-base tree and 0 at this head. It also reports 0 on a trial merge with currentmain(695ab669a), which merges cleanly. - Guard's YAML reading: across all 20 jobs, it extracts the same jobs, steps and compile flags as PyYAML.
- Dependency removal: nothing in the Cloudflare or Spin adapter references
trusted_server_js.cargo metadata --lockedpasses for the root workspace and the integration-tests crate. - Guard edge cases: I ran it against 18 deliberately modified workflow copies. The gaps it showed are in the inline comments.
2 of the inline comments below carry a one-click GitHub
suggestion. Use Commit suggestion (or Add suggestion to batch) to apply them. The other comments describe the fix in prose because it touches several spots or is future work.
Non-blocking
♻️ refactor / 🤔 thinking / ⛏ nitpick / 🌱 seedling
- Compile pattern misses the repo's
_CLI aliases,cargo +toolchain/cargo --flagforms andinstall --path— see inline atscripts/check-ci-node-pin.py:29(suggestion) - A later
setup-nodewith a different version doesn't undo the pin — see inline atscripts/check-ci-node-pin.py:220 ./.tool-versionsand trailing comments are falsely reported as unpinned — see inline atscripts/check-ci-node-pin.py:153(suggestion)- The guard has no tests of its own — see inline at
.github/workflows/format.yml:51
Cross-cutting / body-level findings
- 🏕 List the new guard where the CI gates are documented.
AGENTS.md's "CI Gates" section and.claude/commands/check-ci.mddon't mentionpython3 scripts/check-ci-node-pin.py. A local/check-cirun passes on a step-order regression that CI then rejects. Consider adding it as a gate entry. - 👍 Guard catches exactly the jobs that were wrong. It flags precisely the 8 jobs that compiled core before pinning Node at the merge-base, including all four integration jobs through the shared composite action. Pinning Node inside
setup-integration-test-env, ahead of every Rust build, is the right fix: it coversprepare-artifacts,integration-tests,integration-tests-fastly-ecandbrowser-testsin one place.
CI Status
- integration tests: PASS
- browser integration tests: PASS
- integration tests (Fastly EC lifecycle): PASS
- prepare integration artifacts: PASS
- CodeQL: PASS
- Analyze (rust): PASS
- Analyze (actions): PASS
- Analyze (javascript-typescript): PASS
- Analyze (python): PASS
- cargo test: PASS (required)
- 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
- cargo fmt: PASS (required)
- CI Node pin guard: PASS
- CLAUDE.md symlink guard: PASS
- format-typescript: PASS (required)
- format-docs: PASS (required)
- vitest: PASS
| COMPILE = re.compile( | ||
| r"\bcargo\s+(?:build|check|test|clippy|bench|run|doc|rustc)(?:-[\w-]+)?\b" | ||
| r"|\bworker-build\b" | ||
| ) |
There was a problem hiding this comment.
♻️ refactor — The compile pattern misses the repo's own CLI aliases. .cargo/config.toml defines test_cli_linux, build_cli_*, run_cli_* and install-cli, and trusted-server-cli depends on core, so all of them run the TSJS build. The underscore separator doesn't match (?:-[\w-]+)?\b, so a job that runs cargo test_cli_linux before setup-node passes the guard. It also misses cargo +1.95.0 build, cargo --locked build and cargo install --path crates/trusted-server-cli.
I checked this against modified copies of the workflows: each of those forms passes the current guard when inserted before the Node step in test-cloudflare.
| COMPILE = re.compile( | |
| r"\bcargo\s+(?:build|check|test|clippy|bench|run|doc|rustc)(?:-[\w-]+)?\b" | |
| r"|\bworker-build\b" | |
| ) | |
| COMPILE = re.compile( | |
| r"\bcargo(?:\s+[+-]\S+)*\s+" | |
| r"(?:(?:build|check|test|clippy|bench|run|doc|rustc)(?:[-_][\w-]+)?\b" | |
| r"|install(?:-cli\b|\s.*--path\b))" | |
| r"|\bworker-build\b" | |
| ) |
Verified alone on this head: 0 violations here, still 8 on the merge-base tree. It matches all of the forms above plus the existing cargo build, cargo test-fastly, cargo test --manifest-path … and worker-build lines. It does not match cargo fmt, cargo install viceroy …, cargo metadata, cargo install cargo-nextest or cargo testify.
If you take it, the docstring on lines 13-14 should mention the _ aliases too. Also not covered, and not used by any workflow today: fastly compute build, spin build and wrangler deploy/dev, which compile through their manifest build commands.
| for step in expand_steps( | ||
| root, path, lines, keys["steps"].start, keys["steps"].end, False, frozenset() | ||
| ): | ||
| if step.pins_node: |
There was a problem hiding this comment.
🤔 thinking — Once a job has pinned Node, a later setup-node that installs a different version doesn't undo it. For example, a pinned setup-node, then setup-node with node-version: 20, then cargo build passes the guard, but the TSJS build runs on Node 20. Nothing does this today, but it's the same kind of drift the guard is meant to stop.
One way to handle it: treat every setup-node step as resetting the state to whether that step pins.
# Step dataclass: add
sets_node: bool
# expand_steps, in the Step(...) call: add
sets_node=uses.startswith("actions/setup-node@"),
# check_workflow loop:
if step.sets_node:
pinned = step.pins_node
elif step.compiles and not pinned:Apply this by hand: it touches three separate spots, so it can't be a one-click suggestion. I tried it on this head: 0 violations, still 8 on the merge-base tree, and the re-pin case is now caught. browser-tests still passes because its second setup-node uses the shared step output.
Related, and already noted in the docstring: a node-version taken from any steps.*.outputs.* counts as pinned, without checking that the step it comes from reads .tool-versions.
| if "node-version-file" in options: | ||
| return options["node-version-file"].value.strip("'\"") == ".tool-versions" |
There was a problem hiding this comment.
⛏ nitpick — node-version-file: "./.tool-versions" and node-version-file: .tool-versions # pinned both pin correctly but are reported as unpinned. That fails safe, but the error message would send someone looking for a missing Node step that is actually there.
| if "node-version-file" in options: | |
| return options["node-version-file"].value.strip("'\"") == ".tool-versions" | |
| if "node-version-file" in options: | |
| value = options["node-version-file"].value.partition(" #")[0].strip().strip("'\"") | |
| return value.removeprefix("./") == ".tool-versions" |
Verified alone: 0 violations on this head, still 8 on the merge-base tree, and both forms above now count as pinned. removeprefix needs Python 3.9 or later; ubuntu-latest has 3.12. The node-version: branch below has the same trailing-comment problem, since STEP_OUTPUT is anchored with $.
| # Every job that compiles trusted-server-core also runs the TSJS build, so | ||
| # it must set up the `.tool-versions` Node first (see issue #1204). | ||
| - name: Verify jobs pin Node before compiling Rust | ||
| run: python3 scripts/check-ci-node-pin.py |
There was a problem hiding this comment.
🌱 seedling — The guard has no tests of its own. Its pattern matching and indentation-based YAML reading are where it is weakest, and the alias, re-pin and quoted-path gaps in the other comments all came from small fixture workflows. A follow-up could add a standard-library unittest file with fixture trees and run it in this job. Useful fixtures: compile before pin, conditional pin, pin inside a composite action, a script that compiles, CLI aliases, and a later version change. Run each through --root. Not needed for this PR.
Brings #1225 at c3a33e6. Integration decisions: - .gitignore, AGENTS.md and scripts/README.md keep both sides: #1254's lint-domains gate stays CI gate 10 and #1225's documentation proposal scripts become gate 11; the scripts table keeps #1257's check-ci-node-pin.py row beside #1225's docs-proposal rows. - The lint-domains job from #1254 now uses #1259's setup-rust action and pins Node before cargo, so #1257's check-ci-node-pin gate passes. The two PRs merged cleanly but the combination failed that gate.
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)