Skip to content

fix(rivetkit): bind methods through createWriteThroughProxy#4987

Open
abcxff wants to merge 1 commit into
05-12-feat_rivetkit_add_connectionmap_readonly_map_wrapper_for_actor_connectionsfrom
05-06-fix_rivetkit_bind_methods_through_createwritethroughproxy
Open

fix(rivetkit): bind methods through createWriteThroughProxy#4987
abcxff wants to merge 1 commit into
05-12-feat_rivetkit_add_connectionmap_readonly_map_wrapper_for_actor_connectionsfrom
05-06-fix_rivetkit_bind_methods_through_createwritethroughproxy

Conversation

@abcxff
Copy link
Copy Markdown
Contributor

@abcxff abcxff commented May 6, 2026

Fixes an error that occurs when inspector state attempts to serialize non trivial values (ie Date).

@railway-app
Copy link
Copy Markdown

railway-app Bot commented May 6, 2026

🚅 Deployed to the rivet-pr-4987 environment in rivet-frontend

Service Status Web Updated (UTC)
website 😴 Sleeping (View Logs) Web May 12, 2026 at 2:57 pm
frontend-inspector ❌ Build Failed (View Logs) Web May 12, 2026 at 2:48 pm
frontend-cloud ❌ Build Failed (View Logs) Web May 12, 2026 at 2:47 pm
ladle ❌ Build Failed (View Logs) Web May 12, 2026 at 2:46 pm
kitchen-sink ❌ Build Failed (View Logs) Web May 12, 2026 at 2:45 pm
mcp-hub ✅ Success (View Logs) Web May 6, 2026 at 8:30 pm

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

Code Review: PR #4987 -- fix(rivetkit): bind methods through createWriteThroughProxy

Overview

This PR fixes a serialization bug where inspector state fails to serialize non-trivial values like Date. The root cause: the old hand-rolled Proxy-based createWriteThroughProxy only trapped set and deleteProperty, so mutations via methods (e.g. date.setFullYear(), map.set(), set.add()) were never detected — those go through a get trap to retrieve the method followed by a call, bypassing the set trap entirely.

The fix replaces the hand-rolled proxy with @rivetkit/on-change, which correctly intercepts mutations on Date, Map, Set, TypedArrays, and arrays via method calls. It also introduces a CborSerializable type, assertCborSerializable validation, and adds Set round-trip support to the JSON-compat encoding layer.

The approach is sound and the test coverage is comprehensive.


Issues

1. RegExp was previously allowed but is now rejected

The old isCborSerializable returned true for RegExp (citing CBOR tag 27). The new assertCborSerializable throws for it. This is a breaking state-schema change for any user with RegExp in actor state. If intentional, it should be noted in the PR description.

2. Non-finite number validation regression

The old isCborSerializable rejected NaN and Infinity via !Number.isFinite(value). The new assertCborSerializable passes all typeof value === 'number' values through unconditionally. Encoding NaN will produce output but decoding may silently corrupt state. Recommend adding the guard back:

if (typeof value === 'number') {
    if (!Number.isFinite(value)) {
        throw new TypeError(
            `Value at ${path || 'root'} is non-finite (NaN/Infinity) and is not CBOR serializable`
        );
    }
    return;
}

3. $Set backward-compatibility risk

Any existing serialized actor state shaped like a two-element array starting with "$Set" will now be incorrectly deserialized as a Set instance after this change (same pattern risk as the existing $BigInt/$ArrayBuffer tags). Probably very unlikely in practice, but worth noting.

4. onValidate always returns true

onValidate(_path: string, newValue: unknown) {
    beforeChange?.(newValue);
    return true;
},

The unconditional return true is easy to misread. Rejection works by beforeChange throwing (the test confirms this). A short comment clarifying that rejection is throw-based, not return-value-based, would help future readers.


Minor Notes

  • @rivetkit/on-change is Rivet-scoped. Worth a note in the PR clarifying whether this is a Rivet-maintained fork or the upstream on-change package re-scoped.
  • Pinned version "@rivetkit/on-change": "6.0.1" (no caret) is a reasonable stability choice.
  • The as CborSerializable casts throughout are appropriate since real validation happens at runtime via assertCborSerializable at write paths, and call sites typically hold unknown.
  • Replacing isCborSerializable (boolean check) with assertCborSerializable (throw-on-invalid) aligns with the fail-by-default convention in CLAUDE.md.
  • Test coverage is thorough: all CBOR types, deep nesting, Map/Set/Date/TypedArray method mutations, beforeChange throwing, and Set encoding round-trips. Well done.

Summary

The core fix is correct and well-motivated. The two behavioral regressions (non-finite numbers, RegExp) should be addressed before merge — particularly the NaN/Infinity gap, which is a silent correctness issue rather than a noisy failure.

@abcxff abcxff force-pushed the 05-06-fix_log_error_on_failed_inspector_requests branch from b150344 to fc4756e Compare May 6, 2026 22:03
@abcxff abcxff force-pushed the 05-06-fix_rivetkit_bind_methods_through_createwritethroughproxy branch from af6e0e4 to 3be897d Compare May 6, 2026 22:03
@abcxff abcxff force-pushed the 05-06-fix_rivetkit_bind_methods_through_createwritethroughproxy branch from 3be897d to c39972c Compare May 7, 2026 20:16
@abcxff abcxff force-pushed the 05-06-fix_log_error_on_failed_inspector_requests branch from fc4756e to 5ec0839 Compare May 7, 2026 20:16
@abcxff abcxff marked this pull request as ready for review May 7, 2026 20:19
@abcxff abcxff requested a review from NathanFlurry May 7, 2026 20:23
@abcxff abcxff force-pushed the 05-06-fix_log_error_on_failed_inspector_requests branch from 5ec0839 to 4ff7adb Compare May 11, 2026 03:41
@abcxff abcxff force-pushed the 05-06-fix_rivetkit_bind_methods_through_createwritethroughproxy branch from c39972c to add44af Compare May 11, 2026 03:42
@abcxff abcxff changed the base branch from 05-06-fix_log_error_on_failed_inspector_requests to graphite-base/4987 May 12, 2026 13:21
@abcxff abcxff force-pushed the 05-06-fix_rivetkit_bind_methods_through_createwritethroughproxy branch from add44af to 4455290 Compare May 12, 2026 13:21
@abcxff abcxff changed the base branch from graphite-base/4987 to 05-12-feat_rivetkit_add_connectionmap_readonly_map_wrapper_for_actor_connections May 12, 2026 13:21
@abcxff abcxff force-pushed the 05-06-fix_rivetkit_bind_methods_through_createwritethroughproxy branch from 4455290 to caa9ace Compare May 12, 2026 13:29
@abcxff abcxff force-pushed the 05-12-feat_rivetkit_add_connectionmap_readonly_map_wrapper_for_actor_connections branch from 436c4a6 to b18ec51 Compare May 12, 2026 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant