Skip to content

Set up pinned Node in Rust CI jobs and drop unused tsjs deps - #1257

Open
dhruv8sh wants to merge 8 commits into
mainfrom
chore/ci-pinned-node-tsjs-build
Open

dhruv8sh wants to merge 8 commits into
mainfrom
chore/ci-pinned-node-tsjs-build

Conversation

@dhruv8sh

@dhruv8sh dhruv8sh commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

  • Every Rust build is also a Node build: trusted-server-core depends on trusted-server-js, whose build script runs npm ci / npm run build with the npm on PATH and embeds the bundles. Most Rust CI jobs never set up the .tool-versions Node, so they built those bundles with the runner image's Node (22 on ubuntu-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).
  • Set up the pinned Node, with npm caching, before the first cargo command in every remaining Rust job and in the shared integration-test composite action.
  • Add a CI Node pin guard job that fails when any job compiles Rust before an unconditional setup-node step pinned to .tool-versions, so the ordering can't regress.
  • Drop the unused direct trusted-server-js dependency 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

File Change
.github/workflows/test.yml Add setup-node (node-version-file: .tool-versions, cache: npm) before cargo in test-cloudflare, test-spin, test-parity; name and comment the existing Node steps in test-rust, test-axum, test-cli for the TSJS build
.github/workflows/format.yml Same step before cargo fmt / clippy in format-rust; new check-ci-node-pin job that sets up the .tool-versions Node, fetches only the pre-guard commit, and runs the guard's tests and the guard
scripts/check-ci-node-pin.mjs, scripts/check-ci-node-pin.test.mjs Dependency-free Node guard, run with the .tool-versions Node, that fails when a CI job compiles Rust before an unconditional actions/setup-node step pinned to .tool-versions. It inlines local composite actions and follows repository .sh scripts. Any cargo subcommand except fmt, metadata, tree and registry installs counts as a compile, and so does worker-build. Only a setup-node step that names a version resets the pin; quoted values and trailing comments are handled. The node:test suite (30 tests) covers fixture cases, the current tree and the pre-PR tree
scripts/README.md Document the new script
AGENTS.md, .claude/commands/check-ci.md List the Node pin guard as a CI gate
.github/actions/setup-integration-test-env/action.yml Install the resolved Node right after reading it, before the Fastly/Axum/Cloudflare builds. Covers prepare-artifacts, integration-tests-fastly-ec, browser-tests; a comment notes the step must stay unconditional
.github/workflows/integration-tests.yml Remove the integration-tests job's setup-node step, now done by the composite action
crates/trusted-server-adapter-cloudflare/Cargo.toml Remove unused trusted-server-js dependency
crates/trusted-server-adapter-spin/Cargo.toml Remove unused trusted-server-js dependency
Cargo.lock Drop the two dependency edges

Notes:

Closes

Closes #1204

Spec

Spec: Not a feature

Test plan

  • cargo test-fastly && cargo test-axum
  • cargo clippy-fastly && cargo clippy-axum
  • cargo fmt --all -- --check
  • JS tests: cd crates/trusted-server-js/lib && npx vitest run
  • JS format: cd crates/trusted-server-js/lib && npm run format
  • Docs format: cd docs && npm run format
  • WASM build: cargo build --package trusted-server-adapter-fastly --release --target wasm32-wasip1
  • Manual testing via fastly compute serve
  • Other: cargo check-cloudflare, cargo check-spin, cargo test-cloudflare, cargo test-spin, and clippy for Cloudflare (native + wasm) and Spin (native + wasm). node scripts/check-ci-node-pin.mjs reports 0 violations at this head (8 jobs on the tree before this PR); node --test scripts/check-ci-node-pin.test.mjs passes 30/30. Before the Python version was removed, both gave identical output on both trees and on all 51 test fixtures. actionlint passes on all workflows.

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 localStorage hides 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-versions Node.

Checklist

  • Changes follow AGENTS.md conventions
  • No unwrap() in production code — use expect("should ...") (no Rust code changed)
  • Uses log macros (not println!) (no Rust code changed)
  • New code has tests (CI config, manifest changes and a CI guard script that is exercised by its own CI job)
  • No secrets or credentials committed

…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>
@dhruv8sh dhruv8sh self-assigned this Oct 7, 2026

@ChristianPavilonis ChristianPavilonis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.0 before 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 0002055156cf3acb3eaa604fc045ced11f0adb7c has 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 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread .github/actions/setup-integration-test-env/action.yml Outdated
Comment thread .github/workflows/test.yml
Comment thread .github/workflows/format.yml
Comment thread .github/workflows/test.yml
aram356 and others added 2 commits October 9, 2026 09:41
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 prk-Jr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.py flags exactly 8 jobs on the merge-base tree and 0 at this head. It also reports 0 on a trial merge with current main (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 --locked passes 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 --flag forms and install --path — see inline at scripts/check-ci-node-pin.py:29 (suggestion)
  • A later setup-node with a different version doesn't undo the pin — see inline at scripts/check-ci-node-pin.py:220
  • ./.tool-versions and trailing comments are falsely reported as unpinned — see inline at scripts/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.md don't mention python3 scripts/check-ci-node-pin.py. A local /check-ci run 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 covers prepare-artifacts, integration-tests, integration-tests-fastly-ec and browser-tests in 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

Comment thread scripts/check-ci-node-pin.py Outdated
Comment thread scripts/check-ci-node-pin.py Outdated
Comment thread scripts/check-ci-node-pin.py Outdated
Comment thread .github/workflows/format.yml Outdated
@aram356 aram356 added this to the 202610 milestone Oct 12, 2026
aram356 added a commit that referenced this pull request Oct 12, 2026
aram356 added a commit that referenced this pull request Oct 12, 2026
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.

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Round 2, reviewed at 9f139a0a5. All four round-1 threads are resolved, and I re-checked each one. CI is green, and the job logs show every one of the 12 Rust jobs resolving Node 24.12.0 before its first tsjs: Building line, which covers the second "Done when" item of #1204. The new CI Node pin guard is structurally sound. Its indentation-based parser agrees step for step with a PyYAML parse of every workflow (20 jobs on a trial merge with main, 19 at the merge-base), and it reproduces the 8 violations at the merge-base. In mutation probes it catches every removed or weakened pin: dropped, if:-gated, set to the wrong version, moved after a cargo step, and the composite step gated on build-wasm. What remains is in what it counts as a compile and as a pin, plus some follow-through. prk-Jr's four threads on the script are still open. The comment at lines 29-32 here is an alternative to their line-32 suggestion.

3 of the inline comments below carry a one-click GitHub suggestion. Use Commit suggestion (or Add suggestion to batch for several at once) to apply them as commits on the PR branch. Each was verified alone and all three together: 0 violations on this head and on the trial merge, 8 at the merge-base, and the parser still agrees with PyYAML. The other two inline comments describe changes outside this diff and have to be applied by hand.

Round-1 follow-up

  • setup-integration-test-env comment: the suggested wording was applied verbatim (action.yml:47-51).
  • Older Node steps: renamed for both of their uses (test.yml:56, :103, :282).
  • Step-order check: committed as scripts/check-ci-node-pin.py, run by CI Node pin guard.
  • Local and operator Node pin: filed as #1266 and linked from the description.

Blocking

🔧 wrench

  • Make the compile pattern deny-by-default: see inline at scripts/check-ci-node-pin.py:29-32
  • pins_node reverses setup-node's input precedence: see inline at scripts/check-ci-node-pin.py:153-156
  • An unresolved local action crashes with a traceback: see inline at scripts/check-ci-node-pin.py:170-172
  • List the guard in AGENTS.md and check-ci.md: see inline at .github/workflows/format.yml:42
  • Note on #1266 that its build.rs check retires this guard: see inline at scripts/check-ci-node-pin.py:7
  • Merge main and fix the PR description: see below

Cross-cutting / body-level findings

  • 🔧 Merge main: the branch is 10 commits behind (mergeStateStatus: BEHIND). A trial merge is clean. On the merged tree the guard reports 0 violations and follows main's new scripts/lint-wasm-clock.sh step in format-rust, cargo metadata --locked passes, and neither the Cloudflare nor the Spin adapter references trusted_server_js. The AGENTS.md and check-ci.md lists that the format.yml:42 comment extends also only exist on main.
  • 🔧 PR description: the Notes still say test-axum and test-cli "are unchanged", but 9f139a0 renamed and commented their Node steps, as the Changes table already says. Please drop or reword that sentence.

CI Status

  • integration tests: PASS
  • browser integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • CodeQL: PASS
  • cargo test (ts CLI, native) (macos-latest): PASS
  • format-typescript: PASS (required)
  • CLAUDE.md symlink guard: PASS
  • Analyze (rust): PASS
  • Analyze (actions): PASS
  • CI Node pin guard: PASS
  • cargo test (axum native): PASS
  • cargo test: PASS (required)
  • cargo fmt: PASS (required)
  • cargo check (cloudflare native + wasm32-unknown-unknown): PASS
  • Analyze (javascript-typescript): PASS (two runs)
  • prepare integration artifacts: PASS
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • format-docs: PASS (required)
  • cargo test (cross-adapter parity): PASS
  • cargo test (ts CLI, native) (ubuntu-latest): PASS
  • vitest: PASS
  • Analyze (python): PASS

Comment thread scripts/check-ci-node-pin.py Outdated
Comment thread scripts/check-ci-node-pin.py Outdated
Comment thread scripts/check-ci-node-pin.py Outdated
Comment thread .github/workflows/format.yml
Comment thread scripts/check-ci-node-pin.py Outdated
aram356 added a commit that referenced this pull request Oct 15, 2026
Count any cargo subcommand as a compile except fmt, metadata, tree and registry installs, follow setup-node's precedence of node-version over node-version-file, let a later unpinned setup-node undo a pin, accept ./.tool-versions and trailing comments, and report a missing local action as a ParseError. List the guard in AGENTS.md and the check-ci command.

Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
@dhruv8sh
dhruv8sh requested a review from aram356 October 15, 2026 09:30

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Round 3, reviewed at b967de60b, which is current with main. Every round-2 thread is resolved, mine and prk-Jr's. My three suggestions landed verbatim in 220f86d, along with prk-Jr's ./ normalization and re-pin reset, trailing-comment stripping, AGENTS.md gate 11, check-ci.md step 8 and the note on #1266. The stale description sentence is gone too.

I re-ran the round-2 harness against this head. The guard reports 0 violations here and 8 on the original merge-base tree, and its indentation parser agrees with a PyYAML parse on every job. Every round-2 gap is closed, and every removed or weakened pin is still caught. The macOS system Python (3.9.6) runs it too.

What remains is the Python itself, three false positives in the new parsing code if it stays, and the description.

2 of the inline comments below carry a one-click GitHub suggestion. Use Commit suggestion (or Add suggestion to batch for both at once) to apply them as commits on the PR branch. The third inline comment describes a change across four places of the script, which has to be applied by hand.

Round-2 follow-up (verified at this head)

  • Deny-by-default COMPILE: applied verbatim, docstring updated.
  • node-version precedence: applied verbatim, plus prk-Jr's ./ normalization.
  • ParseError for unresolved local actions: applied verbatim. uses: ./… # comment now resolves.
  • AGENTS.md gate 11 and .claude/commands/check-ci.md step 8: added.
  • #1266: note posted that its build.rs check retires this guard.
  • Merging main and the stale description sentence: done.

Blocking

🔧 wrench

  • Replace the Python guard: see below
  • Quoted value followed by a comment: see inline at scripts/check-ci-node-pin.py:104-106
  • The re-pin reset fires on steps that can't change Node: see inline at scripts/check-ci-node-pin.py:233
  • Docstring line 17 runs to 113 characters: see inline at scripts/check-ci-node-pin.py:17-18
  • PR description: see below

Cross-cutting / body-level findings

  • 🔧 Replace the Python guard: please move this check out of Python. Python isn't part of this repository's toolchain:

    • .tool-versions pins fastly, rust, nodejs, viceroy, wasmtime, awscli and terraform, and the AGENTS.md Toolchain table has no Python row. Yet gate 11 now tells every contributor to run python3.
    • CI runs it on whatever python3 the runner image ships, with no setup-python. That is the same unpinned-tool-on-PATH pattern this PR removes for Node.
    • The floor is already moving: str.removeprefix from 220f86d needs Python 3.9, and the macOS system /usr/bin/python3 is 3.9.6.
    • The repository's other checks are Rust or shell (ts dev lint domains from #733, scripts/lint-wasm-clock.sh from #1226).
    • The indentation-based YAML reader is where each review of this script has found gaps, while serde_yaml_ng 0.10 is already a workspace dependency (used by trusted-server-cli).

    Options, in order of preference:

    1. Implement #1266 option 2 here. In crates/trusted-server-js/build.rs, compare node --version with the nodejs entry in .tool-versions before the TSJS build, and fail on a mismatch, with an override for intentional local use. That check runs in every job that compiles core, so it catches every case this guard does plus the ones it can't see (flow-style steps, scripts reached through $GITHUB_WORKSPACE/… or a working-directory). It covers local and operator builds too. Then delete the guard, as your note on #1266 already proposes.
    2. Port the guard to a small host-only workspace crate that parses the workflows with serde_yaml_ng, with #[cfg(test)] fixtures for the cases the probes in this review cover. That also delivers prk-Jr's deferred test suite. A ts subcommand is the wrong home: trusted-server-cli depends on trusted-server-core, so building it runs the TSJS build, and the guard job would itself need a pinned Node and a full core build.
    3. If it has to stay in Python, state why in the description, and pin it: a .tool-versions entry, an AGENTS.md Toolchain row, and actions/setup-python with that version in the CI Node pin guard job. The three inline comments on the script apply in that case.
  • 🔧 PR description: main's template now has a ## Spec section (#1385), and for a CI change it takes "Spec: Not a feature". The Changes row for scripts/check-ci-node-pin.py also still says it "finds cargo compile commands (aliases included)". It now counts any cargo subcommand except fmt, metadata, tree and registry installs, and it handles re-pins and trailing comments.

Merge coordination (no action needed here)

I trial-merged this head with the other 24 open PRs that touch .github/. The guard passes on every clean merge, including #1259, #1262, #1290, #1260, #1343 and #1387. #1254 conflicts on AGENTS.md gate 11, since both PRs add one. Whichever lands second renumbers (raised on #1254). Eleven draft PRs in the #1345-#1399 stack add dependencies next to the trusted-server-js lines that this PR removes from the Cloudflare and Spin Cargo.toml, so they will get a small adjacent-line conflict once this lands.

CI Status

  • Analyze (actions): PASS
  • Analyze (javascript-typescript): PASS (two runs)
  • Analyze (python): PASS
  • Analyze (rust): PASS
  • browser integration tests: PASS
  • cargo check (cloudflare native + wasm32-unknown-unknown): PASS
  • cargo check/build/test (spin native + wasm32-wasip1): PASS
  • cargo fmt: PASS (required)
  • cargo test: PASS (required)
  • cargo test (axum native): PASS
  • cargo test (cross-adapter parity): PASS
  • cargo test (ts CLI, native) (macos-latest): PASS
  • cargo test (ts CLI, native) (ubuntu-latest): PASS
  • CI Node pin guard: PASS
  • CLAUDE.md symlink guard: PASS
  • CodeQL: PASS
  • format-docs: PASS (required)
  • format-typescript: PASS (required)
  • integration tests: PASS
  • integration tests (Fastly EC lifecycle): PASS
  • prepare integration artifacts: PASS
  • vitest: PASS

Comment thread scripts/check-ci-node-pin.py Outdated
Comment thread scripts/check-ci-node-pin.py Outdated
Comment thread scripts/check-ci-node-pin.py Outdated
Python is not pinned in .tool-versions, so CI ran the guard on whatever python3 the runner shipped. The guard is now a dependency-free Node script run with the .tool-versions Node, and a node:test suite covers it, including the pre-guard tree. Only a setup-node step that names a version resets the pin, and quoted values followed by comments now parse correctly.

Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
@dhruv8sh

Copy link
Copy Markdown
Collaborator Author

@aram356 on the Python guard from your review: moved it to Node in 75c0491, which .tool-versions already pins (24.12.0). scripts/check-ci-node-pin.py is replaced by scripts/check-ci-node-pin.mjs, which uses only the Node standard library. The CI Node pin guard job now runs actions/setup-node with node-version-file: .tool-versions, so no unpinned interpreter is involved. AGENTS.md gate 11, .claude/commands/check-ci.md and scripts/README.md point at the Node commands.

The port keeps the Python behaviour, including all three inline fixes: both versions gave the same output on this head (0 violations), on 7a0ecb4cb (the same 8 jobs) and on all 51 test fixtures before the Python file was removed. A node:test suite (scripts/check-ci-node-pin.test.mjs, 30 tests) runs in the same job.

I kept the guard rather than doing #1266 option 2 here; #1266 can still retire it. The description now has ## Spec and updated Changes rows.

@dhruv8sh
dhruv8sh requested a review from aram356 October 19, 2026 12:11
aram356 added a commit that referenced this pull request Oct 21, 2026

@aram356 aram356 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Round 4, reviewed at 75c049115. The guard is now a dependency-free Node script, scripts/check-ci-node-pin.mjs. The CI Node pin guard job runs it, and its new 30-test node:test suite, on the .tool-versions Node. That settles my round-3 Python concern: Node is pinned, every Rust build already needs it, and node --test scripts are already a CI gate (#1225). All three round-3 inline fixes are in the port, every thread has a reply, and the description has ## Spec and current Changes rows.

The port is faithful. I ran the round-3 Python, with the fixes the port says it carries, against the .mjs over the same 33 mutation probes, and they give identical results. The tests pass 30/30 on Node 24.12.0, 24.20.0 and 26, including the pre-guard tree test with CI=true. The guard reports 0 violations on this head and still 8 at the original base.

What remains:

  • the branch conflicts with main, so no CI has run at this head;
  • a newline false positive in the compile pattern I suggested in round 2;
  • a stale test comment;
  • the CLI's handling of unknown flags.

2 of the inline comments below carry a one-click GitHub suggestion. Use Commit suggestion (or Add suggestion to batch for both at once) to apply them as commits on the PR branch. The third gives its proposed code in a plain block.

Round-3 follow-up (verified at this head)

  • Replace the Python: done, as a Node port that runs on the pinned Node in CI.
  • A quoted value followed by a comment: fixed, with tests for both quote styles and an unterminated value.
  • Re-pin false positives: fixed. Only a step that names a version resets the pin, and a conditional re-pin keeps it. Tests cover all three patterns.
  • Docstring wrap: the header comment now wraps at 80.
  • PR description: ## Spec added, Changes rows updated.

Blocking

🔧 wrench

  • Merge main: see below
  • Newline false positive in the compile pattern: see inline at scripts/check-ci-node-pin.mjs:37-41
  • Stale fetch comment in the pre-guard test: see inline at scripts/check-ci-node-pin.test.mjs:496-500
  • Unknown CLI flags exit like a violation: see inline at scripts/check-ci-node-pin.mjs:397

Cross-cutting / body-level findings

  • 🔧 Merge main: the branch conflicts with main in AGENTS.md and scripts/README.md. Because of that, GitHub hasn't run Run Tests, Run Format or Integration Tests at this head, including the required cargo test, cargo fmt, format-docs and format-typescript checks. Only CodeQL's default setup ran, so the new guard job and its tests haven't run in CI yet.
    • AGENTS.md: #1225 took gate 11 for the documentation proposal scripts. Keep it, and make the guard gate 12. #1254, approved but not merged, also adds a gate 12, so whichever of the two lands second renumbers.
    • scripts/README.md: #1225 added rows and reformatted the table. Keep main's version and add the check-ci-node-pin.mjs row back after benchmark.sh. The result passes the Markdown prettier gate.
    • I resolved it that way in a scratch merge. On that tree:
      • the tests pass 30/30 on main's Node 24.20.0;
      • the guard reports 0 violations over 24 jobs in 7 workflows, including docs-proposal.yml and #1259's setup-rust composite;
      • the parser still agrees with a PyYAML parse;
      • cargo metadata --locked and cargo check -p trusted-server-adapter-cloudflare -p trusted-server-adapter-spin --all-targets pass.
    • The description goes stale with the merge. #1123 has landed and moved the pin to 24.20.0, so "the pinned 24.12.0", "the planned pin bump (#1123)" and the Vitest note under the test plan need updating.

Merge coordination (no action needed here)

A three-way trial merge with #1254 passes the guard (25 jobs). It conflicts only on the list entries both PRs add to AGENTS.md, .claude/commands/check-ci.md and scripts/README.md.

CI Status

  • Analyze (javascript-typescript): PASS
  • Analyze (python): PASS
  • Run Tests, Run Format and Integration Tests workflows: not run (the branch conflicts with main)

Comment on lines +37 to +41
const COMPILE = new RegExp(
String.raw`\bcargo(?:[\s\\]+[+-]\S+)*[\s\\]+` +
String.raw`(?!(?:fmt|metadata|tree|install(?![^\n]*--path))(?:\s|$))\w` +
String.raw`|\bworker-build\b`,
);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 wrench: This is a bug in the pattern I suggested in round 2, and the port carried it over. [\s\\]+ also matches a bare newline. So in a multi-line run: block, the guard reads cargo at the end of one line and the first word of the next line as one command, although bash ends a command at an unescaped newline. This counts as a compile today:

run: |
  cargo --version
  rustc --version

So do command -v cargo and echo cargo followed by any other line. No workflow does this today (main's new setup-rust composite only runs rustup). But a cargo --version sanity print is common CI boilerplate, and the guard would report a missing pin that isn't missing. Allowing only spaces, tabs and a backslash-newline continuation between cargo and its arguments fixes it:

Suggested change
const COMPILE = new RegExp(
String.raw`\bcargo(?:[\s\\]+[+-]\S+)*[\s\\]+` +
String.raw`(?!(?:fmt|metadata|tree|install(?![^\n]*--path))(?:\s|$))\w` +
String.raw`|\bworker-build\b`,
);
const COMPILE = new RegExp(
String.raw`\bcargo(?:(?:[ \t]|\\\n)+[+-]\S+)*(?:[ \t]|\\\n)+` +
String.raw`(?!(?:fmt|metadata|tree|install(?![^\n]*--path))(?:\s|$))\w` +
String.raw`|\bworker-build\b`,
);

Verified alone on this head with Node 24.12.0:

  • 30/30 tests;
  • 0 violations here, and still 8 on the pre-guard tree;
  • the three newline cases above now pass, while cargo \ + newline + check is still flagged.

On a corpus of 27 compile forms (every form from the earlier rounds, plus tab and mixed continuations), it misses none. On 21 lines that must not count, false positives drop from 5 to 0.

Please also add the newline cases to the non-compile list in check-ci-node-pin.test.mjs (lines 164-170). They fail without this fix and pass with it:

    "|\n    cargo --version\n    rustc --version",
    "|\n    command -v cargo\n    echo found",

Comment on lines +496 to +500
// CI fetches full history for this job, so only skip in a local shallow clone.
assert.ok(
!process.env.CI,
`should have ${PRE_GUARD_COMMIT} in CI; check out with fetch-depth: 0`,
);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 wrench: This comment and the assertion message describe a different checkout from the one the job does. The CI Node pin guard job keeps the default depth-1 checkout and then fetches only 7a0ecb4cb…, in its "Fetch the pre-guard commit" step. It doesn't fetch full history, and fetch-depth: 0 isn't how it gets the commit. Someone who drops that step and hits this failure would be told to change the checkout instead of restoring the step.

Suggested change
// CI fetches full history for this job, so only skip in a local shallow clone.
assert.ok(
!process.env.CI,
`should have ${PRE_GUARD_COMMIT} in CI; check out with fetch-depth: 0`,
);
// The CI job fetches this commit before running the tests, so only skip
// in a local shallow clone.
assert.ok(
!process.env.CI,
`should have ${PRE_GUARD_COMMIT} in CI; see the "Fetch the pre-guard commit" step`,
);

Verified alone: prettier-clean, and 30/30 tests with CI=true. With the commit made unreachable, the test still skips locally, and with CI=true it fails with the new message.


/** Run the check from the command line and return the exit code. */
export function main(argv = process.argv.slice(2)) {
const { values } = parseArgs({

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔧 wrench: An unknown flag or a stray positional argument escapes as an uncaught ERR_PARSE_ARGS_UNKNOWN_OPTION with a Node stack trace, and exit code 1. That is the same exit code as a real violation, so a typo such as --rot in a local run or a script reads like a failed check. Catching the parse error gives a one-line message and a distinct exit code. Proposed change for lines 396-403:

export function main(argv = process.argv.slice(2)) {
  let values;
  try {
    ({ values } = parseArgs({
      args: argv,
      options: {
        root: { type: "string" },
        help: { type: "boolean", short: "h" },
      },
    }));
  } catch (error) {
    console.error(`error: ${error.message}`);
    return 2;
  }

Verified on this head: prettier-clean, and 30/30 tests. --rot x now prints error: Unknown option '--rot' and exits 2, and so does a positional argument. The normal run and --help are unchanged. A third case in the command-line test at line 437 would pin the new exit code.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Set up the pinned Node in every Rust CI job and drop the unused trusted-server-js deps

4 participants