Repository navigation
Conversation
The build script shared crates/trusted-server-js/dist with every other cargo build and with manual npm runs, so overlapping builds could embed a partial or empty bundle set and still exit 0. build-all.mjs now accepts --out-dir, and build.rs builds into OUT_DIR/tsjs-dist and fails unless it holds exactly core plus every lib/src/integrations/<id>/index.ts, each non-empty. Silent reuse of dist is gone: TSJS_SKIP_BUILD and a missing npm now fail with instructions, and TSJS_PREBUILT_DIR embeds prebuilt bundles after the same check. Stale node_modules fails instead of reinstalling, npm ci on a missing node_modules is serialized with a file lock, TSJS_TEST failures fail the build, rerun-if-env-changed covers every variable read, and rerun-if-changed is narrowed to the sources. Fixes #1200 Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Cargo reruns a build script on every invocation while a watched path is missing, so always watching lib/node_modules/.package-lock.json made every build rerun under TSJS_PREBUILT_DIR without node_modules. Watch it only on the npm build path, after the freshness check has confirmed it exists. Watch lib/test and lib/vitest.config.ts when TSJS_TEST=1 so edited tests rerun, and note in the error reference that the timestamp-based freshness check also fires when a checkout rewrites an unchanged lockfile. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
The build script now writes bundles to OUT_DIR/tsjs-dist, so the GPT module lookup in template-cache-local-test.sh must search out/tsjs-dist/tsjs-gpt.js. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Summary
The private OUT_DIR bundle builds and completeness validation fix the original concurrency race. Concurrent debug and release builds produced separate complete 13-bundle sets, and an incomplete prebuilt set failed before code generation. I found one medium-severity recovery issue, included inline.
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Summary
Building each Cargo invocation into its own OUT_DIR removes the shared-dist race, and validating the expected bundle set prevents silently embedding missing or empty modules. No blocking findings remain; the two recommendations below improve dependency tracking and regression coverage.
One inline comment includes a one-click GitHub suggestion with the verified replacement.
Non-blocking
♻️ refactor
- Track the external Prebid builder as a JS test input — see inline at
crates/trusted-server-js/build.rs:105.
Cross-cutting / body-level findings
- 🌱 Automate the build regressions. The seven new helper tests cover bundle-set validation. Add regression coverage for simultaneous builds with separate output directories and incremental
TSJS_TEST=1builds after changing the external Prebid builder. Check that both concurrent builds contain the complete non-empty module set and that changing an imported implementation invalidates the requested tests. The current full JS suite creates temporary generated files under watchedlib/src, making unchanged test-enabled builds dirty; the incremental regression should account for that side effect rather than pass because unrelated directory timestamps changed.
Verification
The one-line suggestion passed formatting, all eight target-matched Clippy aliases, Fastly/Axum/Cloudflare compilation checks, all four adapter test aliases, and cross-adapter parity. Rust tests: 3,306 passed, 13 ignored. Broad Rust checks used validated prebuilt bundles; the actual npm build and TSJS_TEST=1 path were also exercised with Node 24.12.0. All 1,185 JS tests and JS formatting passed. Two concurrent JS builds produced all 13 expected non-empty bundles with identical bytes.
The isolated Cargo harness confirms that adding the missing watched input triggers rebuilding. The actual full suite currently changes watched source-directory timestamps on each run, masking the omission, so this is a non-blocking improvement rather than a demonstrated skipped-test regression. The scratch patch remained byte-identical after verification and was discarded.
CI Status
- integration tests (Fastly EC lifecycle): PASS
- integration tests: PASS
- browser integration tests: PASS
- CodeQL: PASS
- cargo test (ts CLI, native): PASS
- Analyze (actions): PASS
- vitest: PASS
- Analyze (javascript-typescript): PASS
- cargo fmt: PASS (required)
- cargo test (axum native): PASS
- Analyze (rust): PASS
- format-typescript: PASS (required)
- cargo check (cloudflare native + wasm32-unknown-unknown): PASS
- Analyze (javascript-typescript): PASS
- format-docs: PASS (required)
- cargo check/build/test (spin native + wasm32-wasip1): PASS
- cargo test (cross-adapter parity): PASS
- cargo test: PASS (required)
- CLAUDE.md symlink guard: PASS
- prepare integration artifacts: PASS
prk-Jr
left a comment
prk-Jr
left a comment
There was a problem hiding this comment.
Approved. Verification found no blocking issues. The dependency-tracking suggestion and regression-test recommendation in the earlier review are non-blocking.
…nal Prebid builder Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
Resolve the semantic conflict with #1180: the generated ALL_MODULE_IDS length now uses the renamed expected module list. Signed-off-by: dhruv8sh <dhruv8sh@proton.me>
ChristianPavilonis
left a comment
ChristianPavilonis
left a comment
There was a problem hiding this comment.
Review summary
Reviewed bcd9b677e3f7c0959ee9474405bc4393bde5d330 against 80483011e30a446ac741b4423e36f8d142c5d2ac. No actionable issues introduced by this PR were found. Inspected all nine changed files and traced bundle generation through Rust metadata, integration selection, static serving, hashes, and browser-test consumers.
Safety proof
- Concurrent debug tests and release compilation produced separate, complete sets of 13 non-empty, byte-identical bundles. Generated hashes matched bundle bytes. Missing, empty, and unexpected prebuilt bundles failed before code generation.
- Fixtures exercised this revision's compiled build script. Two overlapping invocations ran one
npm ciwithout consuming partial dependencies. Two failed installations each cleaned their partial directory, and a third attempt recovered. Failing requested tests, legacy skip, and missing npm all failed closed.
Validation
Checks ran in a temporary snapshot of the exact revision; the original checkout remained clean.
cargo test --package trusted-server-js --target x86_64-unknown-linux-gnu --locked --offline: 10 passed.- Concurrent
cargo build --package trusted-server-js --target x86_64-unknown-linux-gnu --release --locked --offline: passed. cargo clippy --package trusted-server-js --target x86_64-unknown-linux-gnu --all-targets --locked --offline -- -D warnings: passed.npx --no-install vitest run: 1,185 passed; no type errors.npm run format,cargo fmt --all -- --check, andbash -n scripts/template-cache-local-test.sh: passed.- Unchanged normal and prebuilt Cargo rebuilds remained fresh without recompilation.
- Initial snapshot builds rejected the archive's newer lockfile timestamp; preserving the original checkout timestamp cleared the documented freshness check.
All 20 PR checks passed. Existing reviews, comments, replies, and thread resolutions were inspected; both earlier inline findings are fixed and resolved.
Residual validation limits: full adapter/runtime suites were not rerun locally, though their CI checks passed. Installation failure tests used controlled fake npm rather than a network installation.
| // Clean the output directory | ||
| fs.rmSync(distDir, { recursive: true, force: true }); | ||
| fs.mkdirSync(distDir, { recursive: true }); |
There was a problem hiding this comment.
♻️ refactor: --out-dir deletes whatever directory it is given
fs.rmSync(distDir, { recursive: true, force: true }) was safe while distDir was always ../dist. With --out-dir it runs on any path a person types. I ran npm run build -- --out-dir <dir> on a scratch directory holding an unrelated README.md and src/notes.txt; both were deleted and only the 13 bundles remained. Vite refuses to empty an outDir outside the project root unless --emptyOutDir is passed, for the same reason. This script sets emptyOutDir: false and then wipes by hand.
Deleting only tsjs-*.js keeps what the build script relies on (no stale bundle survives, so check_bundle_set still sees exactly this run's output) and leaves everything else alone. Side benefit: npm run build stops deleting dist/prebid/, where build:prebid-external writes by default.
| // Clean the output directory | |
| fs.rmSync(distDir, { recursive: true, force: true }); | |
| fs.mkdirSync(distDir, { recursive: true }); | |
| // Remove only the bundles this script writes, so a wrong --out-dir cannot | |
| // delete unrelated files. | |
| fs.mkdirSync(distDir, { recursive: true }); | |
| for (const name of fs.readdirSync(distDir)) { | |
| if (name.startsWith('tsjs-') && name.endsWith('.js')) { | |
| fs.rmSync(path.join(distDir, name)); | |
| } | |
| } |
Verified with this change: an --out-dir holding unrelated files, a stale tsjs-removed.js and an old tsjs-core.js keeps the unrelated files, drops the stale bundle and replaces core. npm run build still writes 13 bundles to dist, the cargo npm path embeds 13 modules, and npx vitest run (1185 tests) and npm run format pass.
| if let Some(prebuilt_dir) = env::var_os(PREBUILT_DIR_VAR).map(PathBuf::from) { | ||
| println!("cargo:rerun-if-changed={}", prebuilt_dir.display()); |
There was a problem hiding this comment.
♻️ refactor: Reject a relative TSJS_PREBUILT_DIR with a message that says why
Cargo runs build scripts from the package directory, so a relative value resolves against crates/trusted-server-js, not where cargo was invoked. From the repo root, TSJS_PREBUILT_DIR=crates/trusted-server-js/dist cargo build -p trusted-server-js fails with tsjs: failed to read bundle directory crates/trusted-server-js/dist: No such file or directory, naming a path that does exist from the user's side. An empty value fails the same way with a blank path. The documented $PWD/... form works; this makes the obvious relative form fail with an actionable message.
| if let Some(prebuilt_dir) = env::var_os(PREBUILT_DIR_VAR).map(PathBuf::from) { | |
| println!("cargo:rerun-if-changed={}", prebuilt_dir.display()); | |
| if let Some(prebuilt_dir) = env::var_os(PREBUILT_DIR_VAR).map(PathBuf::from) { | |
| assert!( | |
| prebuilt_dir.is_absolute(), | |
| "tsjs: {PREBUILT_DIR_VAR} must be an absolute path because Cargo runs build \ | |
| scripts from {}; got {prebuilt_dir:?}", | |
| crate_dir.display() | |
| ); | |
| println!("cargo:rerun-if-changed={}", prebuilt_dir.display()); |
Verified: cargo fmt --check, clippy-fastly, check-fastly, check-axum and check-cloudflare pass. Relative and empty values now fail with tsjs: TSJS_PREBUILT_DIR must be an absolute path because Cargo runs build scripts from .../crates/trusted-server-js; got "crates/trusted-server-js/dist". An absolute directory and the unset npm path still embed 13 modules.
| if !skip && let Some(npm_path) = npm.as_deref() { | ||
| info!("tsjs: Building per-module bundles"); | ||
| assert!( | ||
| env::var_os(SKIP_BUILD_VAR).is_none(), |
There was a problem hiding this comment.
⛏ nitpick: TSJS_SKIP_BUILD=0 now fails the build
Base skipped only on 1 (is_ok_and(|value| value == "1")), so any other value was a no-op. var_os(..).is_none() fails on any value, including 0 set to mean "build normally". Matching the old trigger keeps the error for the people it targets.
| env::var_os(SKIP_BUILD_VAR).is_none(), | |
| !env::var(SKIP_BUILD_VAR).is_ok_and(|value| value == "1"), |
Verified: fmt and the four lint/check aliases pass. 0, empty and true build normally; 1 fails with the unchanged message, so the error-reference row still matches.
| ```bash | ||
| TSJS_SKIP_BUILD=1 cargo build | ||
| cd crates/trusted-server-js/lib && npm run build && cd - | ||
| TSJS_PREBUILT_DIR="$PWD/crates/trusted-server-js/dist" cargo build |
There was a problem hiding this comment.
⛏ nitpick: Bare cargo build fails at the workspace root
default-members is the Fastly adapter, so bare cargo build links it for the host and fails on undefined Fastly hostcalls after the tsjs step succeeds. I ran this line as written: exit 101. cargo build-fastly builds the same crates for wasm32-wasip1.
| TSJS_PREBUILT_DIR="$PWD/crates/trusted-server-js/dist" cargo build | |
| TSJS_PREBUILT_DIR="$PWD/crates/trusted-server-js/dist" cargo build-fastly |
Verified: the suggested command embeds the 13 prebuilt bundles and finishes, and docs prettier passes.
| bundles into a private directory under its `OUT_DIR` and fails if any expected | ||
| bundle is missing or empty; the `dist` directory is written only by a manual | ||
| `npm run build` (for browser tests) and is never embedded unless you pass it | ||
| explicitly through `TSJS_PREBUILT_DIR`. The external Prebid.js artifact is |
There was a problem hiding this comment.
🏕 camp site: AGENTS.md still describes the old pipeline
This README now says cargo never reads dist, but AGENTS.md, which CLAUDE.md symlinks to and every agent loads, still says it does:
AGENTS.md:326: "Output:dist/tsjs-core.js,dist/tsjs-{integration}.js."AGENTS.md:456: build.rs "Discovers dist files, generatestsjs_modules.rs"
Apply manually, since AGENTS.md is outside this diff. Both lines pass prettier with docs/.prettierrc, and the table width is unchanged:
- Output: `dist/tsjs-core.js`, `dist/tsjs-{integration}.js` from `npm run build`. `build.rs` passes `--out-dir` to build the same files into a private `OUT_DIR/tsjs-dist`, validates the set, and never reads `dist/`.| `crates/trusted-server-js/build.rs` | Builds into `OUT_DIR`, generates `tsjs_modules.rs` || let hidden_modified = fs::metadata(&hidden_lockfile) | ||
| .and_then(|meta| meta.modified()) | ||
| .unwrap_or_else(|_| panic!("{stale_message} ({} missing)", hidden_lockfile.display())); | ||
| assert!(hidden_modified >= lockfile_modified, "{stale_message}"); |
There was a problem hiding this comment.
🌱 seedling: Compare lockfile contents when the timestamps disagree
The error reference documents the false positive: switching to a branch with different dependencies and back rewrites an unchanged package-lock.json, and the build fails until npm ci reinstalls everything. sha2 is already a build dependency, so a follow-up could record sha256(package-lock.json) under node_modules/ whenever this check passes (temp file plus rename, since build scripts run concurrently) and accept a newer lockfile whose hash matches. npm ci deletes node_modules, so the record can't outlive the install it describes.
The timestamp rule itself holds: npm install rewrites package-lock.json and then writes node_modules/.package-lock.json about 60 ms later, so >= is true after both npm ci and npm install. Not for this PR.
This branch has not been deployed
4 participants
Summary
crates/trusted-server-js/dist, whichbuild-all.mjsdeletes and rewrites. Overlapping builds (dev + release in one target dir, two target dirs, or a manualnpm run build) could exit 0 having embedded a partial or empty bundle set, including nocore. The build script now builds into a privateOUT_DIR/tsjs-distand never readsdist.build.rsderives the expected module set the same waybuild-all.mjsdoes (core+ everylib/src/integrations/<id>/index.ts) and fails on any missing, empty or unexpected bundle.distare gone:TSJS_SKIP_BUILDand a missingnpmfail with instructions, and the newTSJS_PREBUILT_DIRembeds prebuilt bundles after the same check.Changes
crates/trusted-server-js/lib/build-all.mjs--out-dir <dir>; default stays../dist, sonpm run build, Playwright and CI are unchangedcrates/trusted-server-js/build/bundle_set.rscheck_bundle_set(missing / empty / unexpected), with unit testscrates/trusted-server-js/build.rsOUT_DIR/tsjs-distand validate before codegen;TSJS_PREBUILT_DIRpath;TSJS_SKIP_BUILDand missingnpmfail with instructions; stalenode_modules(hidden lockfile older thanpackage-lock.json) fails instead of reinstalling;npm cion a missingnode_modulesserialized with a file lock;TSJS_TESTfailures fail the build;rerun-if-env-changedfor every variable read;rerun-if-changednarrowed from ~34k paths to the sources and lockfilescrates/trusted-server-js/src/lib.rsbundle_set.rsunder#[cfg(test)]so its tests run with the cratecrates/trusted-server-js/lib/.gitignorenpm cilock filescripts/template-cache-local-test.shout/tsjs-dist/pathdocs/guide/error-reference.mdTSJS_SKIP_BUILDtip withTSJS_PREBUILT_DIR; document each new build-script errorcrates/trusted-server-js/README.md,docs/guide/creative-processing.mdOUT_DIRanddistis only written bynpm run buildBehavior changes
TSJS_SKIP_BUILD=1now fails; useTSJS_PREBUILT_DIR=<dir with tsjs-*.js>.package-lock.json(for example switching branches), the build asks fornpm ciinstead of building against out-of-date dependencies.node_modulesis still installed automatically (the Axum, Cloudflare, Spin and clippy CI jobs rely on this), but a failednpm cinow fails the build.Coordination
#1180 and #855 also touch the code
build.rsgenerates. This PR changes only theinclude_str!path in that output (/tsjs-dist/tsjs-<id>.js), so rebasing either should be small.Closes
Closes #1200
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 servemainand this branch on the same machinemainnpm run buildloops ondistnode_modulesmissing, second started mid-npm cinpm ciAlso checked by hand:
TSJS_SKIP_BUILD=1, nonpmonPATH, stalenode_modules, and aTSJS_PREBUILT_DIRwith one missing and one empty bundle each fail with the documented message; a validTSJS_PREBUILT_DIRbuilds; a no-change rebuild stays fresh.Checklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)