[npaun]

In our internal repo, we build all our C++ and Rust code with V8's own toolchain. We also rely on V8's GN build, which is more maintained by them.

In workerd, we prefer to be flexible on what toolchains are used and we use V8's Bazel build. How, then, can we get Rust code, like Temporal, building? It turns out it's not too hard, it's just a bit yucky:

1. Patch V8 to expose their Temporal crate in the Bazel build; they haven't added any of the Rust stuff they're doing to Bazel. We could potentially upstream this to them.
2. Add their crates to our build process with a bit of boilerplate.
3. Take the rest of the crates they use straight from crates.io.

Optional follow-ups:

A. We could lock the versions of the crates V8 depends on to match V8. Might prevent trouble in the future? Trivially easy. See #7926.
B. We could add a new hub in rules_rust and let V8 and workerd have their own crate version pins. Clanker assures me this works but I don't like the sound of this.
C. Use V8's vendored crates even for stuff we could get from crates.io. I asked my clanker to implement this and it produced an unholy mess, so I would need to work on this manually, and I don't have bandwidth right now.

Note: this does not enable Temporal and I did a test to prove it won't lead to any mysterious enablement.

I tagged @fhanau as our build expert, and @logan-gatlin because he's dealt with ICU and Rust recently and might have some insight into our best approach.

[clanky-the-clanker]

💬 Approved with comments — No correctness or security problems. There is one documentation WARNING about the V8 update guide, two comment-wording SUGGESTIONs, and an INFO noting that the lockfiles and the V8 patch were not checked line by line.

Findings: 1 warning · 1 info · 2 suggestions

_{The decision is advisory: this comment never approves, blocks or requests changes on the pull request.}

🔧 Fix with your agent

Paste the following into your coding agent to investigate and fix these issues.

Fix the following code review issues found in pull request #7925.

Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
  for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order

## WARNING (1)

### [Documentation] docs/v8-updates.md Step 10 doesn't say to keep the temporal_capi/temporal_rs pins, the Cargo lockfiles and patch 0050 in step with V8 during V8 updates. Without that step, a future update could build Temporal against mismatched crate headers.
- **File:** docs/v8-updates.md:77

## INFO (1)

### [Code Quality] Not checked line by line for correctness: build/deps/v8.MODULE.bazel, deps/rust/Cargo.lock, deps/rust/Cargo.toml, deps/rust/cargo-bazel-lock.json, patches/v8/0050-Build-Temporal-in-the-V8-Bazel-build.patch.

## SUGGESTION (2)

### [Code Quality] Typo in a comment: "temoral_capi" should be `temporal_capi`.
- **File:** deps/rust/BUILD.bazel:47

### [Code Quality] "Temporal is still disabled at runtime for now" describes a temporary state. Say instead that `--noharmony-temporal` in src/workerd/jsg/setup.c++ turns Temporal off.
- **File:** .bazelrc:101

---

📋 Code Quality: 💬 1 info · 2 suggestions

The change works as intended. Two comment-wording nits from the last review are still open, and some files were not checked line by line.

Recommendation: 💬 Optional Nits

• [INFO] Not checked line by line for correctness: build/deps/v8.MODULE.bazel, deps/rust/Cargo.lock, deps/rust/Cargo.toml, deps/rust/cargo-bazel-lock.json, patches/v8/0050-Build-Temporal-in-the-V8-Bazel-build.patch.
• [SUGGESTION] Typo in a comment: "temoral_capi" should be temporal_capi. (deps/rust/BUILD.bazel:47)
• [SUGGESTION] "Temporal is still disabled at runtime for now" describes a temporary state. Say instead that --noharmony-temporal in src/workerd/jsg/setup.c++ turns Temporal off. (.bazelrc:101)

View full review

• deps/rust/BUILD.bazel:47: the comment says "temoral_capi". It should say temporal_capi.
• .bazelrc:101: "Temporal is still disabled at runtime for now" describes a temporary state. The repository's comment guidelines ask comments to describe the current behaviour, for example "Temporal is turned off at runtime by --noharmony-temporal in src/workerd/jsg/setup.c++".
• build/deps/rust.MODULE.bazel, the only file changed since the last review, looks correct. It builds the added BUILD content with "\n".join, and its aliased target matches the @crates_vendor//:temporal_capi_cpp_bindings dependency in deps/rust/BUILD.bazel.
• src/workerd/jsg/setup.c++ negates the flag instead of deleting it and sets it before the configured flags, so v8Flags can still turn Temporal on locally.

This review did not check build/deps/v8.MODULE.bazel, deps/rust/Cargo.lock, deps/rust/Cargo.toml, deps/rust/cargo-bazel-lock.json or patches/v8/0050-Build-Temporal-in-the-V8-Bazel-build.patch line by line for correctness.

---

📝 Documentation: ⚠️ 1 warning

The V8 update guide doesn't cover the Temporal crate pins.

Recommendation: 📝 Minor Comments

• [WARNING] docs/v8-updates.md Step 10 doesn't say to keep the temporal_capi/temporal_rs pins, the Cargo lockfiles and patch 0050 in step with V8 during V8 updates. Without that step, a future update could build Temporal against mismatched crate headers. (docs/v8-updates.md:77)

View full review

Step 10 of docs/v8-updates.md (lines 77-92) lists the V8 dependencies to keep in sync. It already covers the ICU and Rust version coupling, but it doesn't mention temporal_capi / temporal_rs, which deps/rust/Cargo.toml pins to match V8's DEPS. Please add a step to check the crate versions and features against V8's DEPS, regenerate Cargo.lock / cargo-bazel-lock.json, and rebase patch 0050, which edits V8's BUILD.bazel. A pointer from the V8 Updates section of AGENTS.md would also help. Without this step, a later V8 update could build Temporal against mismatched crate headers.

---

✅ All other checks passed

🔒 Security: ✅ LGTM

No security concerns found. temporal_capi and temporal_rs are pinned to =0.2.6 and come from crates.io through the existing Cargo.toml/Cargo.lock flow. Temporal stays off at runtime by default. The crate checksums in the lockfiles were not checked individually.

---

📖 AGENTS.md: ✅ LGTM

The only relevant gap is the V8 update guidance for the Temporal crate pins, which is covered in the documentation section. A short pointer in the V8 Updates section of AGENTS.md would be enough once docs/v8-updates.md covers it.

---

⚡ Performance: ✅ LGTM

The only runtime change is setting one V8 flag once per process at startup. While Temporal is off, none of its code runs on the request path. The cost is extra build time and binary size.

---

_{Reviewed commit: 08d0f09e}

[ask-bonk]

Since last review: 0 resolved, 1 still open, 1 new.

Temporal build wiring now normalizes generated Bazel content across checkout line endings.

Not re-run: api-compat, compat-flags, jsg-gc, kj-style, memory-safety (no author changes in their files since the last review)

---

_{Reviewed commit: 08d0f09e · github run}

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.45%. Comparing base (b986e5c) to head (08d0f09).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7925      +/-   ##
==========================================
- Coverage   69.45%   69.45%   -0.01%     
==========================================
  Files         696      696              
  Lines      181369   181370       +1     
  Branches    29176    29176              
==========================================
- Hits       125966   125965       -1     
  Misses      38359    38359              
- Partials    17044    17046       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[fhanau]

surprisingly clean

[logan-gatlin]

I like option A. I don't like B and C, they seem like way more trouble than they are worth. Right now it doesn't look like V8 patches any of its Rust dependencies it pulls from crates.io. If it ever did in the future, couldn't we just like copy their patches folder into ours?