[suchintan]

https://github.com/Skyvern-AI/rustwright-cloud/pull/233

[claude]

Summary

This is a repo-sync from rustwright-cloud that migrates the Java, C#/.NET, Ruby, and PHP bindings from ad-hoc JSON-tree decoding to a shared, identity-preserving RwWireGraph C-ABI (dense node ids, two-pass allocate-then-link materialization). The design itself is solid — no recursion on the decode path (so cycles/deep nesting are handled safely), borrowed string views are copied by explicit length before rw_wire_graph_free (embedded NULs preserved), and the Rust FFI boundary (capi/src/lib.rs) validates all output pointers and node ids before writing, with no unwrap/panic-prone paths I could find. One real correctness regression in the PHP binding below.

🔴 Critical Issues (1)

• PHP silently drops JS BigInt values returned from evaluate(). In php/src/WireDecoder.php:103-121, decodeLeaf() groups LEAF_BIGINT into the same match arm as LEAF_UNDEFINED/LEAF_SYMBOL/LEAF_FUNCTION and returns null. Every other migrated binding decodes this leaf kind into a real value: Java (WireValueDecoder.java, LEAF_BIGINT -> parseBigIntegerOrString(...)), C# (JsonWire.cs, LeafBigInt => ParseBigIntegerOrString(...)), and Ruby (wire.rb, LEAF_BIGINT → decode_bigint). The capi/src/lib.rs test (bigint_node assertions around line 1007-1012) confirms WireLeaf::BigInt is a real, reachable leaf kind produced by the core parser (e.g. from an unserializableValue ending in n), so this isn't a theoretical wire shape — any PHP page.evaluate() call whose JS expression returns a BigInt will now silently become null instead of an integer/string.
  • Compounding this, php/contract_test.php and the new WireDecoderTest-equivalent case for "bigint" feed {"__rustwright_cdp_unserializable_value__": "123n"} (the generic unserializable leaf, kind 0) rather than the actual bigint leaf tag, and then assert null is correct ("BigInt did not use the PHP null fallback"). That test therefore exercises the wrong code path and never touches LEAF_BIGINT at all — compare with Ruby's contract_test.rb, which correctly uses {"__rustwright_cdp_bigint__": "123n"} and asserts the decoded value is 123.
  • Fix: add a real LEAF_BIGINT arm in decodeLeaf (e.g. parse $fields[0] as int/GMP/fall back to the numeric string, mirroring the other three bindings), and update the contract test to exercise the actual LEAF_BIGINT wire shape.

🟡 Suggestions (2)

• PHP unsigned 64-bit values lose precision. NativeLibrary::wireGraphGetUnsigned() (php/src/NativeLibrary.php) casts overflowing uint64_t values to PHP float, which only has 53 bits of integer precision — a value like 2^60 will silently round. Java and C# instead promote to BigInteger in this case. This is a pre-existing PHP limitation (not introduced by this PR), but since the migration is explicitly meant to bring PHP to parity with the other bindings, it'd be worth using GMP (already implied as available, given bigint handling) or a numeric string instead of a lossy float.
• Per-node FFI round trips. Every binding's materializer issues one native call per node per accessor (kind, then a second call for the typed getter, plus separate array_length/object_length calls in both the allocate and link passes). For very large evaluate() results this means several native calls per node, which is more FFI-boundary crossings than the old single JSON-blob decode. Worth a follow-up benchmark now that a synced parity test exists (bindings/CONTRACT.md) to confirm this net-wins versus the flattened decoder for typical payload sizes — the CONTRACT.md doesn't currently document the two approaches' tradeoffs, and it isn't obvious to a reader which non-identity-preserving bindings (Go, rust-native) still take the simpler path deliberately for a reason.

📝 Minor / Style (2)

• This PR bundles eight separate "🔄 synced local 'X/' with remote 'X/'" bot commits with no per-commit description of what changed inside each directory; the PR body is just a link to the source PR. A brief summary of the substantive change (this migrates 4 bindings' evaluate-decoding onto the new graph ABI) in the PR description would make it much easier to review/bisect later, even though it's an automated sync.
• capi/src/lib.rs and a few of the new NativeBindings/NativeMethods/WireDecoder methods have stray blank-line-heavy diffs (e.g. doubled blank lines after new method blocks in NativeBindings.java, JsonWire.cs, NativeLibrary.php) that add noise without content — a quick pass with the formatter would tidy this up.