fix(sdk): correct i64 parameter legalization and dict key decoding

[kurumpa]

Summary

Fixes the two SDK bugs reported against 0.2.1, both of which corrupted data
silently — no throw, no error object, just a wrong value.

• rf.list([...]) returned RAY_NULL for any non-empty array, and rf.dict({...})
  (which builds its values via list()) produced a dict with null values.
• Dict.toJS() and the dict iterator collapsed every key to "", so an N-key
  dict decoded to a 1-key object.

Bug 1 — int64_t params in non-final position

The build links -s WASM_BIGINT=0, which legalizes every int64_t parameter
into two i32 words (lo, hi). The SDK's cwrap arg lists declare one 'number'
per C parameter, so an int64_t that is not the last parameter shifts every
parameter after it:

ray_t* vec_set_idx(ray_t* obj, int64_t idx, ray_t* val)  // -> (obj, idx_lo, idx_hi, val)

val was consumed as the index's high word, the real val arrived as 0, and
the if (!obj || !val) return RAY_NULL_OBJ; guard fired. The C was correct; the
binding was not.

Fix: a new ray_jsidx_t = double typedef in src/main.c, applied to the
index/length/symbol-id parameters that cross the JS boundary. A double is
passed as one f64, needs no legalization, matches the JS Number domain exactly,
and is exact to 2^53 — far beyond any index reachable in a 4 GiB wasm32 heap.
JS call sites and cwrap arg lists are unchanged.

Chosen over the alternatives: -s WASM_BIGINT=1 would force BigInt at every
cwrap arg list across the whole SDK; int32_t would cap indices at 2^31 for no
benefit over double.

Migrated: init_symbol_str, init_string_str, symbol_to_str, symbol_vec_get,
str_vec_get, init_vector, init_list, vec_at_idx, vec_set_idx,
vec_insert, table_col, table_row, intern_symbol (13 reachable from the
SDK), plus fill_i64_vec / fill_i32_vec / fill_f64_vec, which have no cwrap
and are reachable only by embedders via Module._*.

This also closes a latent truncation: exports whose int64_t was already in
final position appeared to work because the omitted high word defaults to 0, but
silently aliased any index >= 2^32 down to its low word. Index 2**32 used to
overwrite element 0; it now errors.

Silent-corruption hardening

List.set() and List.push() assigned this._ptr = <result> unconditionally,
so a failed op replaced a live list with the null result — which is why this bug
presented as corruption rather than an exception. Both now route through
List._rebind(ptr, op), which throws on a null or error result and leaves the
existing handle intact.

Bug 2 — empty dict keys

Vector.at() on a SYM vector already returns a JS string (it routes through
symbol_vec_get). Dict.toJS() treated it as a symbol id and re-wrapped it:

const keyStr = this._sdk._symbolToStr(Number(keys.at(i)));

Number("x") -> NaN -> symbol_to_str(NaN) -> "". Every key collapsed to
the same empty string and entries overwrote each other, so the last value won.

Fix: use keys.at(i) directly, at both Dict.toJS() and
Dict[Symbol.iterator].

Tests

Adds test-bugs.mjs (23 cases) and a test:bugs script. The cases marked
control pin the paths that were not broken, so a fix that regresses them is
caught. Alongside the public-API assertions, the suite asserts at the raw export
level — the bug was an ABI mismatch, and Vector.at() bounds-checks the
truncation case away before it reaches the boundary. test-bugs.mjs:114
specifically pins that the 4-arg split-index form no longer applies, so a future
WASM_BIGINT change can't silently reintroduce the split.

23/23 passed, 0 failed (bug1: 0, bug2: 0)
npm test: green (smoke + contract)

Not addressed in this MR

Follow-up work, all in the same class as Bug 1 and none of it a regression from
this change:

• i64 return values. Nine exports still return int64_t through a 'number'
  cwrap (read_i64, read_timestamp, read_symbol_id, get_obj_len,
  str_atom_len, get_data_byte_size, intern_symbol, get_cmd_counter,
  table_count). An i64 return is legalized the same way a parameter is, so JS
  gets the low word only: rf.eval('1099511627776').toJS() -> 0 while
  format() is correct. Same double narrowing applies.
• Atom constructors. init_i64 / init_date / init_time / init_timestamp
  keep int64_t in trailing position and truncate at 2^32 —
  rf.i64(2**32+5) -> 5. Timestamps are nanoseconds since 2000, so every real
  value is over the line.
• Double retain in the list bindings. vec_set_idx / vec_push /
  vec_insert call ray_retain(val) before delegating, and ray_list_set /
  ray_list_append / ray_list_insert_at each retain the item again. One extra
  ref per element on success, and an outright leak on the error path. Previously
  unreachable because Bug 1 kept rf.list() / rf.dict() from getting there —
  this MR makes that path the default, so it should land next.
• test:bugs is not wired into npm test. Deliberate for now, pending the
  above; wire it in once the follow-ups land so the suite actually gates.
• Non-SYM dict keys. keys.at(i) is correct for SYM / STR / numeric key
  vectors. Core dicts also permit RAY_LIST (heterogeneous) keys, where at()
  returns a RayObject that stringifies through format(). Needs a
  k instanceof RayObject ? k.toJS() : k coercion.

[singaraiona]

The ABI correction fixes the reported symptoms and the focused regression suite passes, but I found two merge-blocking memory-safety/ownership issues and two additional correctness/coverage gaps. Details are inline.

[singaraiona]

[P1] This now makes the rf.list()/rf.dict() set path reachable, but the list branch below still calls ray_retain(val) before ray_list_set, which retains the item itself. The same double retain exists in vec_push and vec_insert. I reproduced rc 1 -> 3; after dropping both the list and the caller's atom, one reference remained permanently. Please remove the redundant wrapper retains (and release SDK-created temporary wrappers after transferring ownership), otherwise normal list/dict construction leaks every element.

[kurumpa]

Fixed all of them; details below.

The root cause

The engine's list API is borrow semantics — ray_list_append / _set / _insert_at retain the item themselves (src/vec/list.c:107,147), leaving the caller's ref alone. main.c retained as well, so an element went 1 → 3 exactly as you measured: caller's ref, our retain, the list's retain. Dropping the list and the caller's handle returned two, stranding one permanently.

It was dormant before this PR: the mangled i64 index made val arrive as 0, so the list branch was unreachable from rf.list()/rf.dict(). Legalizing the parameter is what exposed it.

Worth noting this isn't a blanket rule — init_dict is correct to retain, because ray_dict_new documents "consumes one ref each." The contract is per-function, so I audited every ray_retain in main.c against its callee rather than removing them by pattern.

C fixes (src/main.c)

The three you flagged:

• vec_set_idx (:604), vec_push (:622), vec_insert (:638) — redundant retain removed from the RAY_LIST branch.

Two more with the identical bug, found by that audit:

• init_table (:727) — ray_table_add_col documents "Retains col_vec internally so the caller keeps its own ref," so the retain stranded one ref per column on every rf.table(). The header comment above it asserted the opposite of the real contract, which is likely how this got in; corrected.
• table_vals (:761) — retained a borrowed column and then passed it to ray_list_append, which retains again: one stranded ref per column on every .values() call, including the ones toJS()/toRows() make internally.

Checked and left alone as already correct: vec_at_idx, dict_keys, dict_vals, dict_get (ray_dict_get is documented as returning an owned ref), table_col, init_dict.

SDK fixes (src/rayforce.sdk.js)

Temporary wrappers, per your second point. List.set/push now drop the wrapper when the SDK minted it for a raw JS value, and leave caller-supplied RayObjects alone. Call sites that pre-converted (dict(), _arrayToVector's mixed path, Table.insert) now pass raw values so List.set owns the temporaries; dict(), table(), and Table.insert drop the intermediates once ownership has transferred.

rf.set() (:618) leaked both its symbol wrapper and, for non-RayObject values, its value wrapper. ray_env_set retains into the binding (env.c:356,377), so both were ours to drop.

Discarded owned handles. These readers each hand back an owned ref; the calling code read one field off it and dropped it on the floor:

Site	Leaked per call
Dict.get() (:1354)	the symbol minted for a string key
Dict.has() (:1372)	the value ray_dict_get returns
Dict.toJS() (:1385) / iterator (:1414)	keys + vals + one per value
List.toJS() (:1221)	one per element
Table.columnNames() (:1445)	the cols vector
Table.toJS() (:1535)	vals + one per column
Table.toRows() (:1560)	one column ref per cell

toRows() was calling this.col(name) inside the row loop; hoisting it out fixes the leak and stops re-resolving each name rows × columns times. Iterators drop their containers in a finally, so abandoning the loop early still releases. Yielded elements remain the consumer's to drop — existing contract, now documented rather than changed.

Vector, StrVector, and RayString are clean — they read through typed arrays or return strings without minting wrappers.

One of these wasn't a leak — it was breaking queries

SelectQuery.execute() bound the table to a fresh __rfq_N global per call and never unbound it. The old comment called this "leave the binding for the caller to clean up," but nothing ever did. The global env is a fixed 1024-slot table (env.c:90), so this pinned every queried table in memory and then failed outright:

query #0 ok, rows=3
query #300 ok, rows=3
query #600 ok, rows=3
query #695 FAILED: name: '__rfq_696' undefined

About 700 queries into a session and the builder stops working. The misleading error is the second half: ray_env_set returned OOM, but rf.set() discarded global_set's return — which on failure is an error block — so a failed bind surfaced later as a nonsense "name undefined" from whatever read it next.

Fixed by adding rf.unset() (global_set with a NULL value hits ray_env_set's documented delete path) and unbinding in a finally; set() now throws on a failed bind instead of swallowing it. execute()'s body moved to _run() so the finally wraps the whole thing.

Two behavior changes worth your attention

1. __rfq_N bindings no longer linger after a query. They were never documented, so nothing should depend on inspecting them — flagging it since it's observable.
2. rf.set() now throws on a failed bind where it previously returned silently.

rf.unset() is a new public method, so it also needed a declaration in rayforce.sdk.d.ts — test-contract.mjs caught that omission on its own.

Tests

Added bug3 (double retain) and bug4 (discarded handles) sections to test-bugs.mjs — 38/38 passing.

I checked every new test is actually diagnostic by reverting the fix in the built output and re-running: all of them fail pre-fix and pass post-fix.

That verification mattered, because my first attempt at the bug4 tests passed against the unfixed SDK. A heap probe is the wrong instrument for those: the readers hand back refs to objects that already exist, so a leaked handle strands a refcount without allocating anything. Those tests now probe the refcount of the underlying object (take a handle, read refCount, drop it), and heap-growth checks are kept only for paths that genuinely allocate (table_keys/table_vals build fresh objects). The bug3 heap tests needed similar calibration — 20k cycles showed nothing even while leaking, since the engine grows the heap in large steps; at 400k cycles the unfixed build goes 68 MB → 320 MB. That stronger version is what caught the residual init_table leak after the first round of fixes.

Full run after a clean make wasm:

• npm run test:bugs — 38/38
• npm test — smoke + contract pass
• npm run test:examples — 15 expression examples, 11 interactive demos, CDN example pass in Chrome

[singaraiona]

[P1] Please validate len_f before narrowing in all three fill_* exports. With this new f64 ABI, len_f == -1 becomes copy_len == -1, and the byte count passed to memcpy wraps to nearly UINT32_MAX on wasm32. A raw _fill_i32_vec(..., -1) probe reached this path; depending on the Emscripten runtime this can overwrite linear memory or trap. Require a finite, integral, non-negative length before calculating the copy size.

[kurumpa]

Confirmed and fixed.

The narrowing

copy_len is int64_t, sizeof(T) is a 32-bit size_t on wasm32. The usual arithmetic conversions make copy_len * sizeof(int64_t) an int64_t of -8, which then truncates to 0xFFFFFFF8 in memcpy's size_t parameter — a ~4 GiB copy from a 64-byte buffer. NaN and Infinity are worse in kind: (int64_t) narrowing of either is undefined behavior, not merely a wrong number.

The fix

Two validators next to the ray_jsidx_t typedef (src/main.c:395-415):

static bool jsidx_to_i64(ray_jsidx_t v, int64_t* out) {
  if (!isfinite(v) || v < 0.0 || v > RAY_JSIDX_MAX || v != floor(v)) return false;
  *out = (int64_t)v;
  return true;
}

RAY_JSIDX_MAX is 2^53−1, the exact-integer ceiling of a double — past that the value being narrowed isn't the value JS meant anyway. The three fill_* exports (:709, :717, :725) now validate before clamping, cast to size_t after, and bail on copy_len <= 0 so a corrupt ray_len(obj) can't produce a negative count either.

Rest of the sweep

The other thirteen sites were the same latent bug. init_vector(type, NaN) was UB, and symbol_to_str(NaN) had already bitten us once — there's a comment at rayforce.sdk.js:1397 about a Number() wrapper producing NaN and the symbol coming back empty.

Each one now fails the way that function already fails for bad input, rather than introducing a new convention:

Site	On an invalid value
init_symbol_str (:429), init_string_str (:435)	ray_error("length", …)
init_vector (:555), init_list (:563)	ray_error("length", …)
vec_set_idx (:648), vec_insert (:683)	ray_error("index", …)
vec_at_idx (:629), table_row (:834), table_col (:823)	RAY_NULL_OBJ — reads like out-of-range
symbol_to_str (:473), symbol_vec_get (:495), str_vec_get (:527)	"" — their existing empty-buffer path
intern_symbol (:898)	-1; interned IDs are non-negative slots

Three notes on the details:

String lengths clamp to the buffer, not just the number. All four string sites are reached through cwrap's 'string' marshaller, which hands us a NUL-terminated copy — so the real extent is knowable and jsidx_to_strlen (:408) validates the number and scans to the NUL. init_string_str(p, 4096) on "hi" now yields "hi" instead of reading 4 KB past the allocation. That's a stronger guarantee than a range check alone, and it costs one pass over a string we're about to copy anyway.

strnlen is unusable here — this TU builds -std=c17, where POSIX names are hidden, so it's open-coded. Worth knowing before someone reaches for it again; my first pass used it and only the real build caught it, since -fsyntax-only without -std=c17 accepts it.

Negative indices still work. The SDK normalizes and bounds-checks them in JS before calling in (rayforce.sdk.js:1113, :1139, :1214, :1243, :1258), so the C layer never legitimately sees one — list.set(-1, …) is unaffected, and its existing test still passes. symbol_vec_get's hand-rolled i < 0 check is now subsumed by the validator.

Tests

New bug5 section in test-bugs.mjs, 11 checks, each looping the full hostile set [-1, NaN, Infinity, -Infinity, 2.5, -0.5, 2^53+2] against the raw exports — which is where these bite, since the SDK's own bounds checks hide them from the JS API. 49/49 passing.

Verified diagnostic the same way as the last round: reverting src/main.c and rebuilding fails 9 of the 11.

The two that pass pre-fix are marked control:. One is the intended happy-path pin. The other is the heap canary, and it's worth being precise about why it passes: on this runtime the wrapped copy traps rather than scribbling, so the abort happens before any damage — which is one of the two outcomes you flagged. A runtime that clamps instead of trapping would corrupt linear memory there rather than throwing. The comment on that check says so, so nobody later reads it as proof the overwrite was harmless.

Ran locally after a clean make wasm: npm run test:bugs 49/49, npm test (smoke + contract) passing. I did not re-run npm run test:examples — that's the browser suite and nothing in this change touches those paths, but say the word if you want it before merge.

Separate defect this turned up — not fixed here

While auditing the parameters I checked the ones still typed int64_t, and four of them have the original bug 1 in full:

rf.i64(1234567890123n)             -> 1912276171          // exactly value mod 2^32
rf.timestamp(800000000000000000n)  -> 1999-12-31T23:59:59.669Z

init_i64, init_date, init_time and init_timestamp each take a bare int64_t. Under WASM_BIGINT=0 that legalizes into two i32 words, and the SDK cwraps each as a single 'number' (rayforce.sdk.js:136-141), so the high word is always zero. Anything ≥ 2^31 truncates — and since timestamps are nanoseconds since 2000 (~8×10^17), every timestamp constructed through the SDK is currently wrong.

I left it alone deliberately, because the ray_jsidx_t trick can't fix it: a double carries integers exactly only to 2^53 ≈ 9×10^15, two orders of magnitude short of the ns range. The real options are an explicit lo/hi parameter pair, passing the value as a string, or turning on WASM_BIGINT=1 — each with a different blast radius on the SDK surface, and none of them a drive-by change to a review fix.

[singaraiona]

[P2] Writing decoded, user-controlled keys into {} still loses a valid __proto__ key (and an object-valued entry changes the result's prototype). I reproduced (dict [__proto__ x] (list 1 2)).toJS() returning only x as an own key. Please build the result with Object.create(null) or use Object.defineProperty so every dict key round-trips.

[kurumpa]

Fixed.

What changed (src/rayforce.sdk.js):

• Added a setOwn(obj, key, value) helper (line 89) that uses Object.defineProperty with writable/enumerable/configurable: true.
• Applied it at all three sites that write user-controlled keys: Dict.toJS() (:1414), Table.toJS() (:1559), Table.toRows() (:1593).

Why defineProperty over Object.create(null): it fixes the same round-trip problem without changing the shape of what the SDK hands back. Null-prototype objects break result.hasOwnProperty(...), print as [Object: null prototype], and would be an observable API change for every existing consumer. With defineProperty the result stays an ordinary object, and __proto__ becomes an own enumerable data property that shadows the inherited accessor.

Verified against the reviewer's exact repro:

(dict [__proto__ x] (list 1 2)).toJS()
  keys: ['__proto__', 'x'], JSON: {"__proto__":1,"x":2}, prototype intact
(dict [__proto__ x] (list (dict [a] (list 1)) 2)).toJS()
  object-valued entry no longer re-points the prototype
(table [__proto__ x] (list [1 2] [3 4]))
  toJS + toRows both keep the column

The table path was reachable too — an engine-built table can carry a __proto__ column even though a JS object literal can't express one on the input side.

Tests: added three cases to the bug2 group in test-bugs.mjs. Confirmed they fail on the pre-fix SDK and pass after; full suite is 52/52, with test.mjs and test-contract.mjs also green. dist/rayforce.sdk.js is synced with src/.

[singaraiona]

[P2] This script is not invoked by npm test, while CI and prepublishOnly only run npm test plus examples. As written, all 23 new regression cases can fail while CI remains green. Please include node test-bugs.mjs in the main test command or add an explicit CI/publish step.

[kurumpa]

Fixed by adding node test-bugs.mjs to the test script.