[npaun]

No description provided.

[ask-bonk]

LGTM

github run

[clanky-the-clanker]

💬 Approved with comments — No correctness, security or performance problems found. The Temporal gating defaults to off, and Temporal.Now now uses the same coarse clock as Date.now(). The only item is an optional documentation suggestion.

Findings: 1 suggestion

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

---

📖 AGENTS.md: 💬 1 suggestion

The JSG guidance could record the new Temporal context-gating and clock invariant.

Recommendation: 💬 Optional Nits

• [SUGGESTION] Document the per-context Temporal gating and the rule that Temporal.Now must read the same coarse clock as Date.now() in src/workerd/jsg/AGENTS.md. (src/workerd/jsg/resource.c++:70)

View full review

src/workerd/jsg/resource.c++ explains that V8 installs Temporal process-wide and that Temporal must read the same coarse clock as Date.now(). A short note in src/workerd/jsg/AGENTS.md would keep this constraint visible for future V8 updates.

---

✅ All other checks passed

🔒 Security: ✅ LGTM

No security issues found. Without the callback, Temporal.Now reads the same coarse clock as Date.now() (the Spectre timer mitigation), and temporal-test.js checks this.

---

📋 Code Quality: ✅ LGTM

enableTemporal defaults to false and the server path sets it from the temporal feature flag. The per-isolate bootstrap context is created through newContext, so the deletion applies there too. The two v8::Context::New calls in io/worker.c++ that bypass newContext are a compile-only dummy context and a throwaway inspector context; neither runs Worker code. Both new tests set their compatibility flags explicitly, so they don't depend on the default compat date.

---

⚡ Performance: ✅ LGTM

No performance issues found.

---

📝 Documentation: ✅ LGTM

No documentation issues found.

---

_{Reviewed commit: d731e3a0}