[suchintan]

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

[claude]

Summary

Solid refactor: it centralizes evaluate-wire decoding in Rust (a WireGraph parser + PyO3/napi graph adapters) instead of seven hand-maintained per-language decoders, and along the way fixes a real prototype-pollution-style bug in the old Node decoder (result[key] = ... on a JSON-derived "__proto__" key would previously mutate the object's prototype instead of setting an own property — the new Object.defineProperty-based path in node/src/lib.rs correctly avoids that, and there's a regression test for it). Error handling at the new boundaries (parse_python_launch_options, napi_error) looks correct — failures become catchable ValueError/Error, not panics. No anti-detection/stealth claims are touched.

🔴 Critical Issues (0)

None found. I specifically checked whether the new recursive WireGraphParser::collect_definitions/build_value (src/lib.rs) could stack-overflow on attacker-controlled evaluate() results (a page can return arbitrarily nested structures). It's protected in practice: serde_json's default RECURSION_LIMIT (128, not disabled via unbounded_depth in either Cargo.toml) caps parse depth before these functions ever recurse that deep, so malformed/deeply-nested input fails with a clean parse error rather than crashing the process.

🟡 Suggestions (2)

• Effective nesting depth dropped from ~1000 to 128. The old Python decoder recursed in Python (recursion limit ~1000, raises catchable RecursionError). The new path goes through serde_json::from_str with its default 128-deep recursion guard, so legitimately deep-but-valid JS object graphs (>128 levels) that used to decode now fail with a decode error. Worth a one-line note in CODE_ARCHITECTURE.md/CONTRACT.md (or bump the limit if a real workload needs more), since this is a quiet behavior change in evaluate()'s contract.
• node/src/lib.rs leaf materialization re-resolves globals per occurrence. leaf_to_unknown calls env.get_global()?.get_named_property_unchecked("Date"/"RegExp"/"URL"/"Error"/"BigInt") for every Date/RegExp/URL/Error/BigInt leaf in a result. For results with many such leaves this is avoidable per-call overhead; caching the constructors once per decode_wire invocation (or per WireMaterializer) would cut redundant property lookups on the hot evaluate() path.

📝 Minor / Style (2)

• python/rustwright/sync_api.py _decode_wire_leaf's regexp branch still falls back to legacy "p"/"f" keys (payload.get("pattern", payload.get("p"))), but the Rust callback (py_wire_leaf) always sends "pattern"/"flags" — the fallback is dead code now and could be dropped for clarity.
• node/test/decode.test.mjs previously stubbed the native binding so decode logic could be unit-tested without a compiled addon; since decoding is now fully native, that isolation is gone and the test requires a built .node binary. Likely an accepted tradeoff of the refactor, but worth confirming CI always builds native before running this suite.

[andrewneilson]

Reviewed the decoding refactor and binding call sites. Found one compatibility regression in Error leaf parsing, detailed inline. Verified the serializer output and previous Node coercion with a focused reproduction; native tests were not run locally because the Rust toolchain and compiled addons are unavailable. This PR is already merged, so this is a comment review.

[andrewneilson]

[P2] Preserve coercion of customized Error metadata

The serializer still emits item.name || "Error", item.message || "", and item.stack || "" without converting them to strings, so this validation rejects values the core itself produces. For example, page.evaluate(() => { const e = new Error("boom"); e.name = 123; return e; }) now fails with wire error field must be a string: name in both Python and Node; previously the adapters returned an Error with the name coerced to "123". An object-valued custom stack has the same problem. Preserve the existing native coercion policy for these fields, or make the serializer/parser contract consistent without losing that behavior, and add a regression covering customized Error metadata.