internal/primordials: Node.js parity, generated and engine-backed - #35567
internal/primordials: Node.js parity, generated and engine-backed#35567robobun wants to merge 26 commits into
Code review found 4 potential issues
Found 5 candidates, confirmed 4. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 4 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | test/js/bun/util/primordials.test.ts:17-31 |
Prelude pre-materializes Map/Set/WeakMap/WeakSet/JSON/Reflect, defeating the 'before any lazy holder exists' premise |
| 🟡 Nit | src/js/internal-for-testing.ts:259-269 |
primordials.refs() is dead code left behind by the e4b4873 test rewrite |
| 🟡 Nit | test/js/bun/util/primordials.test.ts:57 |
AsyncIteratorPrototype holder factory is one prototype hop short |
| 🟡 Nit | test/js/bun/util/primordials.test.ts:213-219 |
Step 3 'structural attacks' are dead: Object.setPrototypeOf/seal/freeze already tampered by steps 1-2 |
Annotations
Check warning on line 31 in test/js/bun/util/primordials.test.ts
claude / Claude Code Review
Prelude pre-materializes Map/Set/WeakMap/WeakSet/JSON/Reflect, defeating the 'before any lazy holder exists' premise
The "pollution planted before any lazy holder exists" test's precondition ("Nothing has touched Map/Set/Date/Math/JSON/... yet in this process") is false for Map/Set/WeakMap/WeakSet/JSON/Reflect: the shared prelude reads `JSON.stringify` / `Reflect.ownKeys` / `Reflect.apply` directly (lines 21, 30-31), and `require("bun:internal-for-testing")` eagerly evaluates `exposedInternals` → `internal/webstreams_adapters` → `internal/primordials`, whose module body runs `makeSafe(Map/Set/WeakMap/WeakSet,
Check warning on line 269 in src/js/internal-for-testing.ts
claude / Claude Code Review
primordials.refs() is dead code left behind by the e4b48731 test rewrite
`primordials.refs()` is now dead code — its only caller (`test/js/bun/util/primordials.test.ts` at 8ea828c8, line 49) was removed when e4b48731 rewrote the test file to use `primordials.audit()` exclusively, and `audit()` already returns `.value` for every primordial so `refs()` is a strict subset with no remaining purpose. Per REVIEW.md ("Delete dead code in the same PR that makes it dead … helpers whose last caller you rewired"), delete the `refs()` method.
Check warning on line 57 in test/js/bun/util/primordials.test.ts
claude / Claude Code Review
AsyncIteratorPrototype holder factory is one prototype hop short
The `AsyncIteratorPrototype` holder factory is one prototype hop short: `getProto(protoOf(() => (async function* () {})()))` yields **%AsyncGeneratorPrototype%** (owns `next`/`return`/`throw`), not **%AsyncIteratorPrototype%** (owns `Symbol.asyncIterator`) — async generators have an extra per-function `.prototype` link that the sync-iterator factories don't. `liveValue` still finds `Symbol.asyncIterator` via inheritance so the identity checks pass, but every tampering test defines/deletes on the
Check warning on line 219 in test/js/bun/util/primordials.test.ts
claude / Claude Code Review
Step 3 'structural attacks' are dead: Object.setPrototypeOf/seal/freeze already tampered by steps 1-2
Step 3's "structural attacks" are dead code: `Object.setPrototypeOf`/`seal`/`freeze` are themselves `ObjectConstructor` primordials, so step 2 has already deleted them from `Object` and planted `poison` on `Function.prototype` at those keys — by the time step 3 runs, `Object.setPrototypeOf(holder, ...)` resolves to `poison`, returns the string `"poisoned"`, and mutates nothing (the `try/catch` hides the no-op). Capture `setPrototypeOf`/`seal`/`freeze` in the prelude alongside `defineProperty` an